-
Notifications
You must be signed in to change notification settings - Fork 4.5k
docs: add danger note to auth guide #35409
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -13,6 +13,11 @@ Regardless of the authentication strategy you choose, you are likely to store au | |||||
|
||||||
We recommend to create `playwright/.auth` directory and add it to your `.gitignore`. Your authentication routine will produce authenticated browser state and save it to a file in this `playwright/.auth` directory. Later on, tests will reuse this state and start already authenticated. | ||||||
|
||||||
:::danger | ||||||
The browser state file may contain sensitive cookies and headers that could be used to impersonate you or your test account. | ||||||
We strongly discourage checking them into private or public repositories. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
::: | ||||||
|
||||||
```bash tab=bash-bash | ||||||
mkdir -p playwright/.auth | ||||||
echo $'\nplaywright/.auth' >> .gitignore | ||||||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love the auth guide to not have a BIG RED WARNING on the first page!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's big red warning worthy, IMO. I don't suggest it lightly and do appreciate not wanting to decorate the docs with a bunch of warnings. However, the consequence of checking these auth files in under many circumstances is full account takeover, and there's evidence many folks are not aware of the risks of checking these in.
That being said, perhaps there's something we could implement to make it safe(r)-by-default.
Some alternative ideas I had (with various degrees of effectiveness vs. implementationd difficulty vs. DX):
Add
playwright/.auth
to the Create Playwright template, and hope folks heed the recommendation to put this files in the.auth/
directory.Have some sort of explicit safelist for headers that the user could configure with everything off-by-default if — and only if — playwright detects the file is not in
.gitignore
.Provide users a specific API for marking sensitive state files and ensure PW puts them in a location outside the tree1:
1: It's important they are put outside the working directory/tree since some folks will decide to by default upload the entire
plawright/*
directory as a build artifact and it would be bad if these files were slurped up into it.wdyt? My vote would be something like #3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dgozman - or perhaps it could be as simple as: