feat: add NETRC support

[karaolidis]

Description

Adds support for a NETRC file. Marked as draft while I test it on my own setup.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Performance improvement
• Code refactoring

Testing

• Tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules

[pchuri]

Thanks for putting this together and for including comprehensive tests and documentation. NETRC support would be a useful addition.

Before merging, could you please address these cases?

1. Quoted NETRC values are currently parsed incorrectly because the parser splits solely on whitespace.
2. A Cloud domain with a trailing slash can be normalized correctly while receiving the wrong /rest/api path.
3. Bearer authentication should match by machine only, even if a stale email remains in the profile.

All 798 tests and lint passed locally. Since this is still a draft, I’d be happy to review it again after the next update. Thanks again!

[karaolidis]

Thanks for your review, great catches on all the points! I've pushed the fixes :)

[pchuri]

Thanks for the quick turnaround! I re-reviewed the updated branch — all three points from the previous review are properly addressed and covered by tests (quoted-value parsing, trailing-slash Cloud domain, bearer machine-only matching with a stale email). All 802 tests and lint pass locally.

While re-reviewing, I found two new issues that I could reproduce by running the code, so I'd like to ask for fixes before merging:

1. # comments in .netrc can hijack an entry

The parser skips unknown tokens as token+value pairs, so a comment containing the word machine creates a phantom entry. Reproduced with:

machine real.example.com
  # my machine token below
  password pw

This parses as [{machine: "real.example.com"}, {machine: "token", password: "pw"}] — the real host's password is absorbed by the phantom entry, so lookupNetrc('real.example.com') returns an entry with no password and the CLI exits with a missing-token error. curl and most implementations skip lines whose first non-whitespace character is #; doing the same in parseNetrc would fix this.

2. normalizeDomain strips context paths from stored domains (regression risk)

getConfig/saveConfig now run the stored domain through normalizeDomain, which removes everything after the first /. An existing on-prem profile like domain: "wiki.example.com/confluence" previously produced https://wiki.example.com/confluence/rest/api and worked; after this PR it is silently truncated to wiki.example.com and requests go to the wrong URL (verified by running getConfig against such a profile). Stripping the scheme is a nice improvement, but I'd suggest keeping the stored domain intact for URL building and only normalizing for the netrc lookup and inferApiPath.

Minor / optional

• Env-path inconsistency: the profile path normalizes the domain but the env path still returns envDomain.trim(), so CONFLUENCE_DOMAIN=cloud.atlassian.net/ yields the right apiPath (per the new test) but a base URL with a double slash (...net//wiki/rest/api).
• Error message: when the token is missing from both the profile and .netrc, the error doesn't mention that a netrc fallback was attempted, which makes netrc typos (machine/login mismatch) hard to debug.
• Dead code: the final .replace(/\/+$/, '') in normalizeDomain is unreachable — the preceding .replace(/\/.*$/, '') already handles it.
• Optional: curl also falls back to .netrc on Windows when _netrc is absent, and warning when the netrc file is group/other-readable (mode & 077) could be worth considering since it holds credentials.

What I liked

• Keeping tokens out of config.json is a real security improvement, and the precedence (env/--token → profile → netrc) is clearly documented.
• Deliberately excluding default entries from the fallback (with a comment explaining why) is a good call — it prevents sending the wrong credentials on a domain typo.
• Solid spec coverage (macdef skipping, case-insensitive host matching) and thorough three-layer tests (parser / lookup / getConfig integration) plus README and SKILL.md updates.

Happy to take another look once items 1 and 2 are addressed — this is very close to mergeable. Thanks again!

[pchuri]

Thanks for the PR — this is a well-built feature. The netrc parser handles macdef bodies, quoted/escaped values, comment lines, and CRLF, the test coverage is thorough (precedence, email mismatch, mTLS exclusion), and the docs are updated accurately. A few findings below.

🔴 Regression: inferApiPath lost case-insensitivity

The old code lowercased the domain before checking .atlassian.net, but the new extractHost() doesn't:

const extractHost = (rawValue) => {
  return (rawValue || '').trim()
    .replace(/^https?:\/\//i, '')
    .replace(/\/.*$/, '');   // no toLowerCase()
};

const inferApiPath = (domain) => {
  const host = extractHost(domain);
  if (host.endsWith('.atlassian.net')) { ... }

A domain like MyCompany.Atlassian.NET used to infer /wiki/rest/api and now falls back to /rest/api. Adding .toLowerCase() inside extractHost fixes this in one line and also keeps netrc matching and API-path inference consistent (lookupNetrc already lowercases internally, so it's unaffected). A test with an uppercase domain → /wiki/rest/api would lock this in.

🟡 Design gap: netrc doesn't work with env-based config

The netrc fallback only runs on the stored-profile path. A curl-style setup — CONFLUENCE_DOMAIN set via env, token in .netrc — doesn't work: with no token, hasEnvAuth is false, so the env path is skipped and config falls through to profile resolution ("No configuration found!" if there's no config file). SKILL.md does say "profiles", so this looks intentional, but the users most likely to want netrc (CI, dotfiles-managed setups) overlap heavily with env-only users. I'd suggest either documenting the limitation in the README or extending the fallback to the env path.

🟡 Parser edge cases

• Mid-line comments: only lines starting with # are skipped. In machine foo # my password here, the # token hits the default case (which unconditionally swallows one following token), and the subsequent password keyword survives — the entry ends up with password='here'. Inline comments aren't standard netrc (curl also only skips line-leading #), but the unconditional i++ in the default case makes the parser fragile against arbitrary trailing text.
• default entries are never matched: recording them with a null host deviates from standard netrc semantics (where default is a fallback). The code comment explains the intent — a one-line note in the README's netrc section would help users who expect curl-like behavior.

🟡 Security suggestion: no file-permission check

The project enforces 0600/0700 when saving its own config (saveConfigFile), but a world-readable .netrc is read silently. Following the ftp/curl convention of warning (or refusing) when the file has group/other read bits would keep the security posture consistent. A POSIX-only fs.statSync(filePath).mode & 0o077 check would suffice.

🔵 Nits

• README precedence wording ("environment variable / --token → profile token → .netrc"): --token is an init-time storage flag rather than a per-request override, and the env token only applies when CONFLUENCE_DOMAIN is also set (pre-existing behavior). Not wrong, just a bit oversimplified.
• Login matching is case-sensitive (entry.login === login) while host matching is case-insensitive. Emails are case-insensitive in practice, so a case mismatch could be confusing to debug.
• extractHost doesn't strip a port, so a domain with :8443 requires the port in the netrc machine too (curl ignores ports when matching). Worth a line in the docs for on-prem users.

Verdict

The inferApiPath case regression is the only thing I'd consider blocking — everything else is optional or documentation. The domain normalization (scheme/trailing-slash stripping) bundled in here is a genuine fix given base URLs are built as ${protocol}://${domain}${apiPath}, and it comes with tests. Nice work overall.

[karaolidis]

Closing this one, sorry. These reviews are LLM-generated, and the issue with asking a model to "review this PR" is that it will always come back with something. Two of the points in your last comment are intentional behavior, not bugs.

I don't have the time to keep revising against that. Feel free to pull the branch and make whatever changes you want before merging; otherwise I'll use the patch locally.

[github-actions]

🎉 This PR is included in version 2.17.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[pchuri]

You're right, and I owe you an apology. The last review was AI-assisted and I posted it without filtering it properly — two of the points flagged behavior you had deliberately designed and documented (the env-only scope and ignoring default entries), and sending a third list after you had already addressed two full rounds of feedback wasn't a fair use of your time.

I didn't want that process failure to bury good work, so I've reopened and merged the PR as-is — it shipped in v2.17.0 with your authorship intact. The only follow-up I applied on main is a one-line change making extractHost lowercase the host, so mixed-case *.atlassian.net domains keep inferring the Cloud API path (1c0455f).

Thanks for the contribution and for the direct feedback — I'll review the reviews before posting them from now on. You'd be welcome back anytime.