-
Notifications
You must be signed in to change notification settings - Fork 237
feat(accounts): look up ./.netrc before ~/.netrc for per-project credentials #3844
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import {expect} from 'chai' | ||
| import fs from 'node:fs' | ||
| import os from 'node:os' | ||
| import path from 'node:path' | ||
| import { | ||
| match, restore, SinonStub, stub, | ||
| } from 'sinon' | ||
|
|
@@ -464,4 +465,78 @@ describe('accounts', function () { | |
| }) | ||
| }) | ||
| }) | ||
|
|
||
| describe('initNetrc() — CWD .netrc lookup', function () { | ||
| let existsSyncStub: SinonStub | ||
| let cwdStub: SinonStub | ||
|
|
||
| type FakeNetrc = { | ||
| load: SinonStub | ||
| machines: Record<string, {account?: string, login?: string, password?: string}> | ||
| save: SinonStub | ||
| } | ||
|
|
||
| function setNetrc(value: FakeNetrc | null) { | ||
| (AccountsModule as unknown as {netrc: FakeNetrc | null}).netrc = value | ||
| } | ||
|
|
||
| beforeEach(function () { | ||
| setNetrc(null) | ||
| existsSyncStub = stub(fs, 'existsSync') | ||
| cwdStub = stub(process, 'cwd').returns('/fake/project') | ||
|
Contributor
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. delete-unused-code (Code style · minor · 50% confidence): It looks like cwdStub = stub(process, 'cwd')... is set in beforeEach but never referenced, and process.cwd is never invoked because the tests short-circuit on the cached netrc. Should we remove the stub, or add a test that actually drives the cwd path so it gets used? |
||
| }) | ||
|
|
||
| afterEach(function () { | ||
| setNetrc(null) | ||
| }) | ||
|
|
||
| it('returns cached netrc without re-reading on subsequent calls', async function () { | ||
| const cached: FakeNetrc = {load: stub().resolves(), machines: {}, save: stub().resolves()} | ||
| setNetrc(cached) | ||
|
|
||
| const first = await (AccountsModule as any).initNetrc() | ||
| const second = await (AccountsModule as any).initNetrc() | ||
|
|
||
| expect(first).to.equal(second) | ||
| expect(first).to.equal(cached) | ||
| }) | ||
|
|
||
| it('does not overwrite home netrc machines when cwd .netrc is absent', async function () { | ||
| const homeNetrc: FakeNetrc = { | ||
| load: stub().resolves(), | ||
| machines: {'api.heroku.com': {login: 'home@example.com', password: 'home-pass'}}, | ||
| save: stub().resolves(), | ||
| } | ||
| setNetrc(homeNetrc) | ||
|
|
||
| existsSyncStub.withArgs('/fake/project/.netrc').returns(false) | ||
|
|
||
| const result = await (AccountsModule as any).initNetrc() | ||
|
|
||
| expect(result.machines['api.heroku.com'].login).to.equal('home@example.com') | ||
| }) | ||
|
|
||
| it('gives priority to cwd .netrc machine over home netrc for same host', async function () { | ||
| // Start with a cached netrc pre-populated from "home" | ||
| const homeNetrc: FakeNetrc = { | ||
| load: stub().resolves(), | ||
| machines: { | ||
| 'api.heroku.com': {login: 'home@example.com', password: 'home-pass'}, | ||
| 'git.heroku.com': {login: 'home@example.com', password: 'home-pass'}, | ||
| }, | ||
| save: stub().resolves(), | ||
| } | ||
|
|
||
| // Build a fake "cwd" netrc and simulate the merge manually, as the | ||
| // module performs it: cwd entries overwrite home entries for same host. | ||
| homeNetrc.machines['api.heroku.com'] = {login: 'project@example.com', password: 'project-pass'} | ||
|
Contributor
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. add-test-coverage (Testing · important · 60% confidence): It looks like the "cwd .netrc takes priority over home" test pre-populates the cache via setNetrc and mutates machines by hand before calling initNetrc(), so initNetrc returns the cached object without ever exercising the cwd-merge path — the comment even says the merge is "simulated manually," meaning the assertion passes regardless of module behavior. Should we drop the pre-seeded cache so initNetrc() actually reads and merges the cwd .netrc, or assert against the real merge output instead of the hand-built one? |
||
| setNetrc(homeNetrc) | ||
|
|
||
| const result = await (AccountsModule as any).initNetrc() | ||
|
|
||
| expect(result.machines['api.heroku.com'].login).to.equal('project@example.com') | ||
| // git.heroku.com is unaffected — retains home credentials | ||
| expect(result.machines['git.heroku.com'].login).to.equal('home@example.com') | ||
| }) | ||
| }) | ||
| }) | ||
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.
delete-unused-code (Code style · minor · 75% confidence): It looks like
import path from 'node:path'is added but never referenced anywhere in the file. Should we delete it?