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.

Sign up for GitHub

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

Jump to bottom

Don't leak GitHub tokens when pushing #13507

Merged
merged 1 commit into from Oct 4, 2016
Merged

Don't leak GitHub tokens when pushing #13507

merged 1 commit into from Oct 4, 2016

Conversation

aneeshusa
Copy link
Contributor

@aneeshusa aneeshusa commented Sep 29, 2016 •
edited by larsbergstrom

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because they just remove output/were lightly tested by hand

If git is unable to resolve the repo address (which includes the token),
it will print a message to stderr with the path to the repo, thus
leaking the token. Avoid doing this.

This change is Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 29, 2016
@aneeshusa
Copy link
Contributor Author

r? @larsbergstrom

@aneeshusa aneeshusa mentioned this pull request Sep 29, 2016
Closed
@larsbergstrom
Copy link
Contributor

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 4562598 has been approved by larsbergstrom

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 29, 2016
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Sep 29, 2016
@aneeshusa aneeshusa changed the title Don't leak github tokens during network failures Use more-secure file-based credential storage for github tokens Sep 29, 2016
@aneeshusa
Copy link
Contributor Author

Since the queue is pretty long, I've updated this to a more secure method. r? @SimonSapin

One thing I'm unsure of is whether there should be a colon (:) between the token and the @github.com. We didn't have it before but the example you linked does, so I added it in case the file-based storage works differently.

@highfive highfive assigned SimonSapin and unassigned larsbergstrom Sep 29, 2016
@SimonSapin
Copy link
Member

Looks ok to me, though I’ve never used credential.helper and don’t know what was the previous method that @larsbergstrom had approved.

Does this need corresponding changes on the CI servers?

@aneeshusa aneeshusa changed the title Use more-secure file-based credential storage for github tokens Don't leak GitHub tokens when pushing Oct 4, 2016
@aneeshusa
Copy link
Contributor Author

I spent some time looking at the git credentials documentation and I couldn't really understand how it worked, so I've moved away from that in the interest of knowing how our scripts work. This is now using the previous method of simply redirecting stdout/stderr, which is simpler and well-understood.

@aneeshusa
Don't leak GitHub tokens during network failures
630b523 
If git is unable to resolve the repo address (which includes the token),
it will print a message to stderr with the path to the repo, thus
leaking the token. Avoid doing this, and also suppress stdout to be
extra careful.
@SimonSapin
Copy link
Member

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 630b523 has been approved by SimonSapin

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 4, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit 630b523 with merge d067f46...

bors-servo pushed a commit that referenced this pull request Oct 4, 2016
Auto merge of #13507 - aneeshusa:avoid-leaking-github-tokens, r=Simon…
d067f46 
…Sapin

Don't leak GitHub tokens when pushing

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because they just remove output/were lightly tested by hand

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

If git is unable to resolve the repo address (which includes the token),
it will print a message to stderr with the path to the repo, thus
leaking the token. Avoid doing this.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13507)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

@bors-servo bors-servo merged commit 630b523 into servo:master Oct 4, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@SimonSapin SimonSapin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@aneeshusa @larsbergstrom @bors-servo @SimonSapin @Manishearth @highfive