Skip to content

feat: add support for nonce#10

Merged
DylanPiercey merged 6 commits intomarko-js:mainfrom
snyamathi:cspNonce
Nov 3, 2022
Merged

feat: add support for nonce#10
DylanPiercey merged 6 commits intomarko-js:mainfrom
snyamathi:cspNonce

Conversation

@snyamathi
Copy link
Copy Markdown
Contributor

@snyamathi snyamathi commented Nov 3, 2022

This PR adds a CSP nonce if given in $global. fixes: #8

Description

The midstream redirect script should include a CSP nonce if one exists in $global

Since we don't have a handle to $global in the redirect function, I can't really think of a better way to add the nonce in without either

  • changing the signature of res.redirect (not ideal)
  • adding a new variable to req or res which might break others

Motivation and Context

We need a CSP Nonce for our security settings

Screenshots (if appropriate):

Checklist:

  • I have updated/added documentation affected by my changes.
  • I have added tests to cover my changes.

cc @DylanPiercey


I signed the CLA but still showing an error

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Nov 3, 2022

🦋 Changeset detected

Latest commit: 6043795

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@marko/express Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Nov 3, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

Comment thread src/redirect.ts Outdated
(this.getHeader("Content-Type") as string | undefined)?.startsWith(
"text/html"
)
export function createRedirectWithMidstreamSupportFn({ cspNonce }: { cspNonce?: string; } = {}) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adds a wrapper to the function so we can inject the csp nonce

Comment thread src/redirect.ts Outdated
return redirectWithMidstreamSupport;
}

export const redirectWithMidstreamSupport = createRedirectWithMidstreamSupportFn();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Default export to maintain backward compatibility and avoid having to re-create the function each request unless a nonce is given.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 3, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (8af3945) compared to base (5672b65).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 8af3945 differs from pull request most recent head 6043795. Consider uploading reports for the commit 6043795 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #10   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines           53        58    +5     
  Branches        13        16    +3     
=========================================
+ Hits            53        58    +5     
Impacted Files Coverage Δ
src/index.ts 100.00% <100.00%> (ø)
src/redirect.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@DylanPiercey
Copy link
Copy Markdown
Contributor

DylanPiercey commented Nov 3, 2022

@snyamathi thanks for the reminder and the PR. I went ahead and improved/simplified a few things and will get out the patch shortly 😄

(Note: there's something wrong with the format script in the CLI, I went ahead and ensured the formatting was good)

@snyamathi
Copy link
Copy Markdown
Contributor Author

Got it - yeah I like the approach using a symbol to store the cspNonce on the res object - LGTM and much easier.

@DylanPiercey
Copy link
Copy Markdown
Contributor

Awesome. I'm just working to figure out what's wrong with the format CI. Once I get that fixed I'll get this merged.

@DylanPiercey
Copy link
Copy Markdown
Contributor

Seems like github just doesn't like having a commit action from a fork so 🤷. Thanks again for the PR, just going to go ahead and merge!

@DylanPiercey DylanPiercey merged commit 88cd7b0 into marko-js:main Nov 3, 2022
@github-actions github-actions bot mentioned this pull request Nov 3, 2022
@snyamathi snyamathi deleted the cspNonce branch November 3, 2022 23:07
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.

res.redirect script tag should include nonce

2 participants