Skip to content

[Remove] test-only external URL predicate - #436

Open
juanmaguitar wants to merge 1 commit into
trunkfrom
juanmaguitar/cleanup-428
Open

[Remove] test-only external URL predicate#436
juanmaguitar wants to merge 1 commit into
trunkfrom
juanmaguitar/cleanup-428

Conversation

@juanmaguitar

Copy link
Copy Markdown
Collaborator

Why

isAllowedExternalUrl() only restated whether normalizeExternalUrl() returned a value and existed solely for tests. Keeping that duplicate API made the tests assert an implementation wrapper instead of URL normalization behavior.

What changes

Remove the test-only predicate from src/external-url.js. The former tests now assert normalizeExternalUrl()'s returned normalized URL or refusal directly, while existing opening coverage remains intact.

How to test this

Platforms: any — this is a pure URL-normalization module with no platform-specific branch.

Starting state:

  1. Check out this branch with dependencies installed.
  2. Run node --test tests/unit/external-url.test.cjs.

Expected result: the focused suite passes, including parsed-scheme normalization and refused file: URLs.

What must not have happened: invalid or non-HTTP(S) input must not reach the openExternal recorder; normalized HTTP(S) URLs must still be the values handed to it.

No visible UI surface changed; the focused test command demonstrates this internal cleanup.

Risks and limitations

The removed export was intentionally test-only; a downstream consumer importing it would now fail, but repository-wide search found none. Self-review: 0 findings across the five required dimensions.

Related

Fixes #428.


Design decisions and alternatives considered

The assertions were folded into direct normalization checks rather than deleted wholesale, retaining coverage of the parser-derived protocol and output that callers actually use.

Review outcome (required — see AGENTS.md)

0 [fix here] · 0 [follow-up] — no findings across architecture, security, performance, cross-platform behavior, or tests.

Implementation notes

Validation: node --test tests/unit/external-url.test.cjs (11 passing), npm run lint, and npm test (1,258 passing). The unrelated pre-existing package-lock.json modification was left uncommitted.

@juanmaguitar juanmaguitar added the audit-code Follow-up work from the codebase audit label Sep 10, 2026
@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 67c1c12e-f6ae-4137-81ba-da4ef7fb559c

📥 Commits

Reviewing files that changed from the base of the PR and between fd83488 and d2fdff9.

📒 Files selected for processing (2)
  • src/external-url.js
  • tests/unit/external-url.test.cjs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@juanmaguitar

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audit-code Follow-up work from the codebase audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove the test-only external URL eligibility predicate

1 participant