Skip to content

Add url-params-must-encode lint rule#31

Open
danielchen0 wants to merge 2 commits intomainfrom
add-url-params-must-encode-rule
Open

Add url-params-must-encode lint rule#31
danielchen0 wants to merge 2 commits intomainfrom
add-url-params-must-encode-rule

Conversation

@danielchen0
Copy link
Collaborator

Summary

  • Adds url-params-must-encode rule that flags template literal interpolations in URL query parameter positions not wrapped in encodeURIComponent()
  • Detects ?key=${value} and &key=${value} patterns
  • Allows encodeURIComponent() wrapping and nested wrapping

Test plan

  • All existing tests pass
  • 7 new tests pass
  • TypeScript compiles
  • Prettier passes

Copy link
Contributor

@arnavsurve arnavsurve left a comment

Choose a reason for hiding this comment

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

Clean, well-structured PR. The rule logic is correct, tests cover the main scenarios, and integration follows existing conventions. A couple of minor observations below, but nothing blocking.

🤖 Generated with Claude Code

}

// Also allow String() or toString() wrapping encodeURIComponent inside
if (t.isCallExpression(expr)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: The comment says "Also allow String() or toString() wrapping encodeURIComponent inside" but toString() (a method call, i.e. expr.toString()) is not actually handled here -- only function-call wrapping like String(encodeURIComponent(...)) is detected. The code is fine, but the comment is slightly misleading. Consider updating it to just say "Also allow function calls wrapping encodeURIComponent, e.g. String(encodeURIComponent(...))".

const results = lint(code);
expect(results).toHaveLength(0);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: There's no test for the nested wrapping allowance path (lines 32-40 of the rule), e.g.:

it('allows String(encodeURIComponent(...)) wrapped values', () => {
  const code = 'const url = `https://api.example.com?q=${String(encodeURIComponent(query))}`;';
  const results = lint(code);
  expect(results).toHaveLength(0);
});

Worth adding to ensure that code path stays covered.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants