WIP: rewrite using classes #47
Conversation
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
packages | 1279d4a | Commit Preview URL Branch Preview URL |
Jul 29 2025, 07:50 PM |
commit: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @mcansh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request initiates a major refactor of the http-helmet package, transitioning its core functionality to a class-based architecture. This rewrite introduces a new, more modular v2 API, alongside significant dependency cleanup and enhancements to the monorepo's build and testing configurations, aiming for improved maintainability and extensibility.
Highlights
- Core Refactor to Class-Based API: The
http-helmetpackage is undergoing a significant rewrite, introducing newContentSecurityPolicy,StrictTransportSecurity, andSecurityHeadersclasses under av2module. This shifts the API from a functional approach to an object-oriented one, allowing for more structured and extensible header management. - Dependency Management & Cleanup: Several unused or outdated dependencies have been removed (e.g.,
cli-highlight,marked,yargs), and some core utilities likecontent-security-policy-parserandts-extrashave been added as direct dependencies where needed. Build tooling dependencies like@arethetypeswrong/clihave been updated to@arethetypeswrong/core. - Build System Enhancements: The
tsdown.config.tsfiles forhttp-helmetandurlpackages have been updated to include newv2entry points, configureattw(Are The Types Wrong) directly, and defineimport.meta.vitestfor better build process and type checking. - Test Suite Restructuring: The
mergeHeaderstests have been moved to a dedicatedutils.spec.tsfile, and new comprehensive test suites for thev2class-based APIs (content-security-policy.spec.ts,helmet.ts,strict-transport-security.ts) have been added, ensuring the new architecture is well-covered.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
==========================================
+ Coverage 93.80% 93.89% +0.08%
==========================================
Files 19 23 +4
Lines 872 1114 +242
Branches 215 276 +61
==========================================
+ Hits 818 1046 +228
- Misses 54 68 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant work-in-progress refactoring to a class-based approach for the http-helmet package, creating a new v2 API. The changes are extensive, touching everything from the core logic to build configurations and dependencies. The new class-based API is a great step towards a more modern, fluent interface.
I've identified a few high-severity correctness issues in the new ContentSecurityPolicy class that could lead to invalid headers or unexpected behavior. I've also found a medium-severity configuration issue that would affect documentation generation and another that could impact tooling. Addressing these points will improve the robustness and maintainability of the new API.
| public toString(): string { | ||
| return Array.from(this.#policy.entries()) | ||
| .map(([key, values]) => { | ||
| if (key === "upgrade-insecure-requests") { | ||
| return "upgrade-insecure-requests"; | ||
| } | ||
|
|
||
| if (typeof values === "boolean") return; | ||
|
|
||
| return `${key} ${values.join(" ")}`; | ||
| }) | ||
| .join("; "); | ||
| } |
There was a problem hiding this comment.
The toString() method can produce an invalid Content Security Policy string with extra semicolons (e.g., directive1; ; directive2). This happens when the .map() callback returns undefined for certain directives (like boolean-valued ones parsed from a string), and Array.prototype.join preserves these as empty slots.
To fix this, you should filter out falsy values from the array before joining.
| public toString(): string { | |
| return Array.from(this.#policy.entries()) | |
| .map(([key, values]) => { | |
| if (key === "upgrade-insecure-requests") { | |
| return "upgrade-insecure-requests"; | |
| } | |
| if (typeof values === "boolean") return; | |
| return `${key} ${values.join(" ")}`; | |
| }) | |
| .join("; "); | |
| } | |
| public toString(): string { | |
| return Array.from(this.#policy.entries()) | |
| .map(([key, values]) => { | |
| if (key === "upgrade-insecure-requests") { | |
| return "upgrade-insecure-requests"; | |
| } | |
| if (typeof values === "boolean") return; | |
| return `${key} ${values.join(" ")}`; | |
| }) | |
| .filter(Boolean) | |
| .join("; "); | |
| } |
| append(key: CspDirective, values: CspValueForKey<CspDirective>): this { | ||
| let existing = this.#policy.get(key) || []; | ||
|
|
||
| if (typeof existing === "boolean" || typeof values === "boolean") { | ||
| throw new ContentSecurityPolicyError( | ||
| `Cannot append to boolean directive: ${key}`, | ||
| ); | ||
| } | ||
|
|
||
| let definedValues = this.#getDefinedValues(values); | ||
|
|
||
| this.#policy.set(key, [...existing, ...definedValues]); | ||
|
|
||
| return this; | ||
| } |
There was a problem hiding this comment.
The append method doesn't correctly handle the upgrade-insecure-requests directive. This directive is a boolean flag and doesn't take values, so appending to it is not a valid operation. The current implementation could lead to unexpected behavior.
You should add a check to prevent appending to this specific directive and guide the user to use the upgradeInsecureRequests() method instead.
append(key: CspDirective, values: CspValueForKey<CspDirective>): this {
if (key === "upgrade-insecure-requests") {
throw new ContentSecurityPolicyError(
"Cannot append to 'upgrade-insecure-requests' directive. Use the `upgradeInsecureRequests()` method instead.",
);
}
let existing = this.#policy.get(key) || [];
if (typeof existing === "boolean" || typeof values === "boolean") {
throw new ContentSecurityPolicyError(
`Cannot append to boolean directive: ${key}`,
);
}
let definedValues = this.#getDefinedValues(values);
this.#policy.set(key, [...existing, ...definedValues]);
return this;
}| @@ -1,3 +1,3 @@ | |||
| { | |||
| "entryPoints": ["./src/index.ts", "./src/react.tsx"] | |||
| "entryPoints": ["./src/index.ts", "./src/react.tsx", "./src/index.v2.ts"] | |||
There was a problem hiding this comment.
There appears to be a typo in the entry point for the v2 API. The path is specified as ./src/index.v2.ts, but based on the project structure and tsdown.config.ts, it should be ./src/v2/index.ts.
This will cause typedoc to fail when generating documentation for the new API.
"entryPoints": ["./src/index.ts", "./src/react.tsx", "./src/v2/index.ts"]
| @@ -0,0 +1,101 @@ | |||
| import { SELF } from "#src/utils.ts"; | |||
There was a problem hiding this comment.
The import path "#src/utils.ts" uses a path alias that doesn't seem to be configured for this package in package.json. While this might work in your test environment, it can cause issues with tooling and build processes that don't know how to resolve it.
It's recommended to use a relative path to make the import resolution explicit and robust.
| import { SELF } from "#src/utils.ts"; | |
| import { SELF } from "../utils.ts"; |
1 participant
No description provided.