Skip to content

security: add SecureHeaders pipe to default web pipeline#1390

Open
renich wants to merge 2 commits into
amberframework:masterfrom
renich:sentinel-secure-headers-121991005817310114
Open

security: add SecureHeaders pipe to default web pipeline#1390
renich wants to merge 2 commits into
amberframework:masterfrom
renich:sentinel-secure-headers-121991005817310114

Conversation

@renich

@renich renich commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Introduces a new SecureHeaders middleware pipe to default-enable basic HTTP security headers (XSS Protection, Frame Options, nosniff, HSTS) in new generated web apps.

Co-developed-by: Gemini AI renich+gemini@woralelandia.com
Signed-off-by: Rénich Bon Ćirić renich@woralelandia.com

Co-authored-by: renich <225115+renich@users.noreply.github.com>
@renich renich force-pushed the sentinel-secure-headers-121991005817310114 branch from ca893ac to d9fc791 Compare June 15, 2026 10:48
@crimson-knight

Copy link
Copy Markdown
Member

Good direction — shipping baseline security headers out of the box is something we want. The pipe itself is clean and the chosen low-risk headers are sensible (notably no CSP, and X-Frame-Options: SAMEORIGIN rather than DENY). Two changes needed before we can merge.

1. It's never wired in. The PR adds src/amber/pipes/secure_headers.cr and a spec, but nothing plugs the pipe into a pipeline — grep -rn SecureHeaders only matches the new pipe and its spec. As-is it's defined-but-inert dead code, so generated apps get no headers. (Your own .jules/sentinel.md says the fix belongs in routes.cr.ecr — that file just wasn't touched.)

Since Amber v1 still ships its own CLI generators, the wiring belongs in the app template: add plug Amber::Pipe::SecureHeaders.new to the web pipeline in src/amber/cli/templates/app/config/routes.cr.ecr (alongside CSRF/Session/Flash/Logger). Please also add an assertion that the pipe is actually present in the generated/default pipeline, not just tested in isolation — that's the gap that let the missing wiring slip through.

2. Don't ship HSTS on by default. Strict-Transport-Security: max-age=31536000; includeSubDomains is the one dangerous default here: once a browser sees it over HTTPS it will force HTTPS for the domain and all subdomains for a year, which breaks HTTP dev/staging and HTTP-only subdomains and is hard to undo. Please make HSTS opt-in (commented out in the template with a note, or gated behind a config flag), while keeping X-Content-Type-Options, X-Frame-Options, and Referrer-Policy on by default. (X-XSS-Protection is a no-op in modern browsers — fine to keep or drop.)

Keeping the .jules note is fine. With the pipe wired into the template + HSTS made opt-in + a pipeline-inclusion assertion, this is good to go.

Plug SecureHeaders into the default web pipeline template. Turn HSTS off
by default (making it opt-in) to prevent staging/development breaks,
and add Referrer-Policy by default. Add assertion in pipelines_spec
verifying the pipe is actually present in the default web pipeline.

Co-developed-by: Gemini AI <renich+gemini@woralelandia.com>
Signed-off-by: Rénich Bon Ćirić <renich@woralelandia.com>
@renich

renich commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

I have updated the PR to address your feedback.

Following your suggestions:

  1. I have wired the SecureHeaders pipe into the generated app routes template (src/amber/cli/templates/app/config/routes.cr.ecr) under the :web pipeline.
  2. I added a pipeline-inclusion assertion in spec/amber/cli/commands/pipelines/pipelines_spec.cr to verify that SecureHeaders is indeed present in the default web pipeline.
  3. I disabled HSTS by default in the pipe itself (defaulting the hsts constructor parameter to false), making it opt-in as requested. I also added Referrer-Policy (defaulting to strict-origin-when-cross-origin) to the default security header set.
  4. I updated the secure_headers_spec.cr file to cover both the HSTS-off and HSTS-on paths.

All tests compile and pass.

(Note: This contribution was co-developed with Gemini AI. Rénich has directed, reviewed, tested, and takes full responsibility for this code.)

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