Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: httpOnly param #41

Merged
merged 1 commit into from
Nov 14, 2022
Merged

feat: httpOnly param #41

merged 1 commit into from
Nov 14, 2022

Conversation

tsop14
Copy link
Contributor

@tsop14 tsop14 commented Nov 6, 2022

Issue # (if available):
None

Description of changes:

Adds httpOnly boolean property to Authenticator params.
If true set cookies using HttpOnly.
These cookies can still be read from the request in the @edge lambda but are no longer available to javascript Document.cookie API.

  • adds httpOnly param
  • conditionally sets cookies with HttpOnly
  • adds unit test
  • updates README's param documentation

Context

This issue discusses "Tokens exposed to users via cookies: security question" . It notes to avoid cookies you could follow the authorization code grant flow - requiring a request to cognito to ensure code is valid each time.

There is a tradeoff between latency and security.

We'd like to avoid this extra call but also have additional security by setting the cookies using HttpOnly as a way to mitigate attacks involving cookies.

For our use case we don't need to use amplify on the frontend and this PR assumes support isn't a requirement?
But it might be a bit confusing as the cookies are set with the prefix expected by amplify auth and the architecture diagram includes the amplify logo by the web app. Tried to avoid confusion by making it explicit in readme but happy for feedback on making this clearer if needed.

Thanks for reviewing.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jeandek jeandek added the added-feature For PRs which containg a new feature (may be in response to a `feature-request`) label Nov 8, 2022
@jeandek jeandek added this to the 1.2.3 milestone Nov 8, 2022
@jeandek
Copy link
Contributor

jeandek commented Nov 8, 2022

Hi @tsop14,

While support for Amplify is a requirement for this package, not all of its features need to be compatible as long as they remain optional. Adding the ability to set the HttpOnly flag on cookies is within the scope of the package.

I will try to review your CR by the end of the week (probably Friday).

Cheers,

@jeandek jeandek requested a review from borisfba November 10, 2022 16:47
@borisfba borisfba merged commit 096f3ee into awslabs:main Nov 14, 2022
@tsop14 tsop14 deleted the feat/http-only branch November 14, 2022 19:53
@borisfba borisfba modified the milestones: 1.2.3, 1.3.0 Dec 5, 2022
@ckifer ckifer mentioned this pull request Dec 5, 2022
borisfba pushed a commit that referenced this pull request Dec 13, 2022
* Support SameSite directive similar to feat: httpOnly param #41
* Add type for SameSite
* Set SameSite using existing pattern
* Unit test for addition and for incorrect value validation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
added-feature For PRs which containg a new feature (may be in response to a `feature-request`)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants