fix(affiliates): reject non-HTTP destination URLs#723
Conversation
Greptile SummaryThis PR introduces a shared
Confidence Score: 4/5The core fix is correct and well-tested, but two adapters outside the PR's explicit scope — awin and digistore24 — still pass unvalidated destinationUrl values to external APIs using the same redirect-encoding pattern the PR is designed to eliminate. The nine adapters touched in this PR are correctly hardened, and the shared parseHttpUrl helper is a clean abstraction. However, a survey of the full adapter set reveals that packages/affiliates/awin/src/index.ts (line 46, body.destinationUrl = destinationUrl) and packages/affiliates/digistore24/src/index.ts (line 126, fallback_url: destinationUrl) forward the caller-supplied URL to external APIs without a protocol check — the same gap that triggered this fix. packages/affiliates/awin/src/index.ts and packages/affiliates/digistore24/src/index.ts each encode destinationUrl into an outbound API request without the HTTP protocol guard added in this PR. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[getTrackingLink called\nwith destinationUrl] --> B{destinationUrl\nprovided?}
B -- No --> C[Skip validation\npass empty/null to API]
B -- Yes --> D[parseHttpUrl]
D --> E{new URL\nparses?}
E -- No --> F[throw: must be\nan absolute URL]
E -- Yes --> G{protocol is\nhttp: or https:?}
G -- No --> H[throw: must use\nHTTP or HTTPS]
G -- Yes --> I[Return URL object]
I --> J[Adapter builds\ntracking link]
subgraph "Not yet guarded"
K[awin: body.destinationUrl = destinationUrl]
L[digistore24: fallback_url: destinationUrl]
end
Reviews (3): Last reviewed commit: "fix(affiliates): validate remaining deep..." | Re-trigger Greptile |
| export function parseHttpUrl(value: string, label = 'URL'): URL { | ||
| let url: URL; | ||
| try { | ||
| url = new URL(value); | ||
| } catch { | ||
| throw new Error(`${label} must be an absolute URL`); | ||
| } | ||
| if (url.protocol !== 'http:' && url.protocol !== 'https:') { | ||
| throw new Error(`${label} must use HTTP or HTTPS`); | ||
| } | ||
| return url; | ||
| } |
There was a problem hiding this comment.
Three redirect-style adapters still encode non-HTTP destination URLs
parseHttpUrl is correctly applied to 9 adapters in this PR, but skimlinks, everflow, and refersion share the exact same redirect-encoding pattern and were not updated:
packages/affiliates/skimlinks/src/index.tsline 44:url.searchParams.set('url', destinationUrl)—destinationUrlis set directly as the redirect targeturlquery parameter with no protocol check (same pattern as the now-fixed Rakuten/Sovrn/Tradedoubler/ShareASale adapters).packages/affiliates/everflow/src/index.tswithFallbackDestination:parsed.searchParams.set('url', destinationUrl)— same issue.packages/affiliates/refersion/src/index.tswithDestinationSubId:url.searchParams.set('u', destinationUrl)—destinationUrlis encoded as a sub-parameter on the referral link with no validation.
A javascript:alert(1) or data: URL passed to any of those three adapters today will be embedded in the returned tracking link, bypassing the fix introduced here.
|
Addressed the remaining adapter coverage in Validation: all nine affected affiliate suites pass (101 tests); Admitad, CJ, and Impact typechecks and builds pass; |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
1b42e1e to
d8b2aba
Compare
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
13 similar comments
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
Summary
parseHttpUrlhelper that accepts only absolute HTTP/HTTPS URLsWhy
new URL()accepts schemes such asjavascript:,data:,mailto:, andftp:. The affected adapters previously treated those values as valid destinations; eBay could emit a direct non-web URL, while redirect-style adapters could encode one into a tracking link.Fixes #722.
Validation
git diff --checkpassed