Potential fix for code scanning alert no. 43: Server-side request forgery#985
Open
aaspinwall wants to merge 2 commits into
Open
Potential fix for code scanning alert no. 43: Server-side request forgery#985aaspinwall wants to merge 2 commits into
aaspinwall wants to merge 2 commits into
Conversation
…gery Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…typecheck
The Copilot autofix broke the build and runtime:
- `normalizeApiPath` was a private method on ApiConnection, which is used
structurally as a mock type across stores/tests; a private member breaks that
structural compatibility, failing `tsc` (frontend-tests-and-lint).
- Its per-segment allowlist rejected query strings (`.../links?type=file`),
trailing slashes (`sharing/${hash}/`) and email path params
(`users/lookup/${email}/`), and percent-encoded segments — breaking real
endpoints (end-to-end tests).
Replace it with a module-level `buildApiUrl(serverUrl, path)` that rejects empty
and absolute/protocol-relative paths, strips leading slashes, resolves against
the server origin via `new URL('/api/' + path, serverUrl)`, and asserts the
resulting origin equals the configured origin before use. This closes the SSRF
vector (the host can never be steered by `path`) while preserving every existing
path unchanged. Adds unit tests for the guard and for `call()`'s URL building.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
aaspinwall
marked this pull request as ready for review
July 10, 2026 17:33
Collaborator
Author
|
This is ready for review but low priority |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Potential fix for https://github.com/thunderbird/tbpro-add-on/security/code-scanning/43
The best fix is to normalize and validate API paths before building the URL, and to construct URLs via
new URL()+ pathname joining rather than raw string concatenation.Concretely in
packages/send/frontend/src/lib/api.ts:Add a private sanitizer method on
ApiConnectionthat:http://,https://,//),/and validates each segment with a conservative allowlist (e.g. alphanumerics,_,-,.),/.In
call(...), replaceconst url = \${this.serverUrl}/api/${path}`;`const refreshTokenUrl = \${this.serverUrl}/api/auth/refresh`;with URL-construction usingnew URL()` and the sanitized path:const safePath = this.normalizeApiPath(path);const url = new URL(/api/${safePath}, this.serverUrl).toString();const refreshTokenUrl = new URL('/api/auth/refresh', this.serverUrl).toString();This preserves existing functionality for valid paths like
uploads/${id}/statwhile preventing malicious path content from steering requests to unintended endpoints.Suggested fixes powered by Copilot Autofix. Review carefully before merging.