Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check SimpleFIN access key format #485

Merged
merged 5 commits into from
Nov 3, 2024
Merged

Check SimpleFIN access key format #485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ade5f8c
check SimpleFIN access key format
psybers Oct 26, 2024
7cce296
add release note
psybers Oct 26, 2024
37d9218
fix linter
psybers Oct 26, 2024
851f99d
Merge branch 'master' into sf-key-accesskey
psybers Oct 26, 2024
86a48e3
Merge branch 'master' into sf-key-accesskey
psybers Nov 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/app-simplefin/app-simplefin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,10 @@ function parseAccessKey(accessKey) {
let username = null;
let password = null;
let baseUrl = null;
if (!accessKey || !accessKey.match(/^.*\/\/.*:.*@.*$/)) {
console.log(`Invalid SimpleFIN access key: ${accessKey}`);
throw new Error(`Invalid access key`);
}
Comment on lines +269 to +272
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance error messages for better debugging

The current error message "Invalid access key" doesn't provide enough context about what specifically is wrong with the format.

Consider providing more specific error messages:

-    throw new Error(`Invalid access key`);
+    throw new Error('Invalid access key format. Expected format: https://username:[email protected]');
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!accessKey || !accessKey.match(/^.*\/\/.*:.*@.*$/)) {
console.log(`Invalid SimpleFIN access key: ${accessKey}`);
throw new Error(`Invalid access key`);
}
if (!accessKey || !accessKey.match(/^.*\/\/.*:.*@.*$/)) {
console.log(`Invalid SimpleFIN access key: ${accessKey}`);
throw new Error('Invalid access key format. Expected format: https://username:[email protected]');
}

⚠️ Potential issue

Security: Remove sensitive data from error logs

The current implementation logs the full access key which contains credentials. This poses a security risk as credentials could be exposed in log files.

Apply this change to remove sensitive data from logs:

-    console.log(`Invalid SimpleFIN access key: ${accessKey}`);
+    console.log('Invalid SimpleFIN access key format');
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!accessKey || !accessKey.match(/^.*\/\/.*:.*@.*$/)) {
console.log(`Invalid SimpleFIN access key: ${accessKey}`);
throw new Error(`Invalid access key`);
}
if (!accessKey || !accessKey.match(/^.*\/\/.*:.*@.*$/)) {
console.log('Invalid SimpleFIN access key format');
throw new Error(`Invalid access key`);
}

🛠️ Refactor suggestion

Strengthen the access key validation

The current regex pattern (^.*\/\/.*:.*@.*$) is too permissive and could allow malformed URLs. Consider these improvements:

  1. Validate scheme is 'http://' or 'https://'
  2. Ensure username and password are not empty
  3. Validate the base URL format

Consider applying this improved validation:

-  if (!accessKey || !accessKey.match(/^.*\/\/.*:.*@.*$/)) {
+  if (!accessKey || !accessKey.match(/^https?:\/\/[^:]+:[^@]+@[^/]+$/)) {
     console.log('Invalid SimpleFIN access key format');
     throw new Error(`Invalid access key`);
   }
+  // Additional validation after splitting
+  [scheme, rest] = accessKey.split('//');
+  if (!['http:', 'https:'].includes(scheme)) {
+    throw new Error('Invalid scheme: must be http or https');
+  }
+  [auth, rest] = rest.split('@');
+  [username, password] = auth.split(':');
+  if (!username || !password) {
+    throw new Error('Missing credentials');
+  }
+  if (!rest || !rest.includes('.')) {
+    throw new Error('Invalid base URL');
+  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!accessKey || !accessKey.match(/^.*\/\/.*:.*@.*$/)) {
console.log(`Invalid SimpleFIN access key: ${accessKey}`);
throw new Error(`Invalid access key`);
}
if (!accessKey || !accessKey.match(/^https?:\/\/[^:]+:[^@]+@[^/]+$/)) {
console.log('Invalid SimpleFIN access key format');
throw new Error(`Invalid access key`);
}
// Additional validation after splitting
[scheme, rest] = accessKey.split('//');
if (!['http:', 'https:'].includes(scheme)) {
throw new Error('Invalid scheme: must be http or https');
}
[auth, rest] = rest.split('@');
[username, password] = auth.split(':');
if (!username || !password) {
throw new Error('Missing credentials');
}
if (!rest || !rest.includes('.')) {
throw new Error('Invalid base URL');
}

[scheme, rest] = accessKey.split('//');
[auth, rest] = rest.split('@');
[username, password] = auth.split(':');
Expand Down
6 changes: 6 additions & 0 deletions upcoming-release-notes/485.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: Enhancements
authors: [psybers]
---

Check if SimpleFIN accessKey is in the correct format.