starred/App-Store-Connect-CLI

Fork 0

mirror of https://github.com/rudrankriyam/App-Store-Connect-CLI.git synced 2026-04-25 07:35:48 +03:00

[GH-ISSUE #521] ASC_BYPASS_KEYCHAIN: empty env value should not implicitly enable bypass #145

New issue

Closed

opened 2026-02-26 21:33:46 +03:00 by kerem · 3 comments

kerem commented

2026-02-26 21:33:46 +03:00

Owner

Originally created by @rudrankriyam on GitHub (Feb 14, 2026).
Original GitHub issue: https://github.com/rudrankriyam/App-Store-Connect-CLI/issues/521

Summary

shouldBypassKeychain() treats an empty-but-set ASC_BYPASS_KEYCHAIN value as true, enabling bypass unexpectedly.

Why this matters

A shell/session with export ASC_BYPASS_KEYCHAIN= (empty) silently changes credential resolution behavior away from keychain.

Current behavior

In internal/auth/keychain.go:

if trimmed == "" {
    return true
}

Expected behavior

Empty value should be treated as unset/false, or require explicit truthy values only.

Reproduction

export ASC_BYPASS_KEYCHAIN=
asc auth status

Current behavior reports keychain bypass path.

Detailed implementation plan

Update shouldBypassKeychain() semantics so empty string does not imply bypass.
Keep explicit false values (0, false, no, off) as false.
Keep explicit true values (1, true, yes, on) as true.
Add tests for empty, unset, truthy, and falsey cases.
Update docs/help text to clarify accepted values.

Acceptance criteria

Empty ASC_BYPASS_KEYCHAIN no longer enables bypass.
Explicit true/false env values behave predictably.
Credential resolution warnings/status reflect corrected semantics.

cc @cursor please implement this end-to-end.

Originally created by @rudrankriyam on GitHub (Feb 14, 2026). Original GitHub issue: https://github.com/rudrankriyam/App-Store-Connect-CLI/issues/521 ## Summary `shouldBypassKeychain()` treats an empty-but-set `ASC_BYPASS_KEYCHAIN` value as `true`, enabling bypass unexpectedly. ## Why this matters A shell/session with `export ASC_BYPASS_KEYCHAIN=` (empty) silently changes credential resolution behavior away from keychain. ## Current behavior In `internal/auth/keychain.go`: ```go if trimmed == "" { return true } ``` ## Expected behavior Empty value should be treated as unset/false, or require explicit truthy values only. ## Reproduction ```bash export ASC_BYPASS_KEYCHAIN= asc auth status ``` Current behavior reports keychain bypass path. ## Detailed implementation plan - [ ] Update `shouldBypassKeychain()` semantics so empty string does not imply bypass. - [ ] Keep explicit false values (`0`, `false`, `no`, `off`) as false. - [ ] Keep explicit true values (`1`, `true`, `yes`, `on`) as true. - [ ] Add tests for empty, unset, truthy, and falsey cases. - [ ] Update docs/help text to clarify accepted values. ## Acceptance criteria - [ ] Empty `ASC_BYPASS_KEYCHAIN` no longer enables bypass. - [ ] Explicit true/false env values behave predictably. - [ ] Credential resolution warnings/status reflect corrected semantics. cc @cursor please implement this end-to-end.

kerem

2026-02-26 21:33:46 +03:00

closed this issue
added the
bug
label

kerem commented

2026-02-26 21:33:47 +03:00

Author

Owner

@rudrankriyam commented on GitHub (Feb 14, 2026):

@cursor please implement this issue end-to-end using the checklist and acceptance criteria.

@rudrankriyam commented on GitHub (Feb 14, 2026): @cursor please implement this issue end-to-end using the checklist and acceptance criteria.

kerem commented

2026-02-26 21:33:47 +03:00

Author

Owner

@cursor[bot] commented on GitHub (Feb 14, 2026):

Implemented end-to-end on cursor/asc-bypass-keychain-empty-2971 and pushed as commit 5a10bda (fix(auth): require explicit truthy ASC_BYPASS_KEYCHAIN).

What I changed

Fixed bypass semantics in internal/auth/keychain.go:
- ASC_BYPASS_KEYCHAIN now bypasses keychain only for explicit truthy values: 1, true, yes, on.
- Explicit falsey values remain false: 0, false, no, off.
- Empty/unset/whitespace/invalid values now do not bypass.
Added tests for the full matrix in internal/auth/keychain_test.go:
- unset, empty, whitespace, truthy, falsey, invalid.
- Verified RED first (failing on empty/whitespace/invalid), then GREEN after fix.
Updated status/warning/help text to reflect corrected semantics:
- Removed hardcoded ASC_BYPASS_KEYCHAIN=1 messaging in auth status + doctor.
- Clarified accepted values in command help and README.md.
Added behavior tests for messaging:
- internal/cli/auth/auth_test.go: truthy non-1 (yes) triggers bypass warning and avoids hardcoded =1.
- internal/auth/doctor_test.go: doctor storage message supports truthy non-1 and avoids hardcoded =1.

Validation performed

Targeted RED test (before fix):
go test ./internal/auth -run TestShouldBypassKeychainEnvSemantics -count=1 (failed as expected).
Targeted GREEN tests (after fix):
go test ./internal/auth -run 'TestShouldBypassKeychainEnvSemantics|TestDoctorStorageBypassMessageSupportsTruthyEnvValues' -count=1
go test ./internal/cli/auth -run 'TestAuthStatusCommand|TestAuthLoginCommand' -count=1
Required full checks:
- make format ✅
- make lint ✅ (I rebuilt golangci-lint with Go 1.26 to match repo target)
- make test ✅
Black-box binary check:
- go build -o /tmp/asc .
- Captured output artifact: /opt/cursor/artifacts/asc_bypass_keychain_semantics.txt
- Shows:
  - unset + empty => Credential storage: System Keychain
  - truthy yes => Credential storage: Config File + bypass warning with accepted truthy values.

@cursor[bot] commented on GitHub (Feb 14, 2026): Implemented end-to-end on `cursor/asc-bypass-keychain-empty-2971` and pushed as commit `5a10bda` (`fix(auth): require explicit truthy ASC_BYPASS_KEYCHAIN`). ### What I changed - **Fixed bypass semantics** in `internal/auth/keychain.go`: - `ASC_BYPASS_KEYCHAIN` now bypasses keychain **only** for explicit truthy values: `1`, `true`, `yes`, `on`. - Explicit falsey values remain false: `0`, `false`, `no`, `off`. - **Empty/unset/whitespace/invalid values now do not bypass**. - **Added tests for the full matrix** in `internal/auth/keychain_test.go`: - unset, empty, whitespace, truthy, falsey, invalid. - Verified RED first (failing on empty/whitespace/invalid), then GREEN after fix. - **Updated status/warning/help text** to reflect corrected semantics: - Removed hardcoded `ASC_BYPASS_KEYCHAIN=1` messaging in auth status + doctor. - Clarified accepted values in command help and `README.md`. - **Added behavior tests for messaging**: - `internal/cli/auth/auth_test.go`: truthy non-`1` (`yes`) triggers bypass warning and avoids hardcoded `=1`. - `internal/auth/doctor_test.go`: doctor storage message supports truthy non-`1` and avoids hardcoded `=1`. ### Validation performed - Targeted RED test (before fix): `go test ./internal/auth -run TestShouldBypassKeychainEnvSemantics -count=1` (failed as expected). - Targeted GREEN tests (after fix): `go test ./internal/auth -run 'TestShouldBypassKeychainEnvSemantics|TestDoctorStorageBypassMessageSupportsTruthyEnvValues' -count=1` `go test ./internal/cli/auth -run 'TestAuthStatusCommand|TestAuthLoginCommand' -count=1` - Required full checks: - `make format` ✅ - `make lint` ✅ (I rebuilt `golangci-lint` with Go 1.26 to match repo target) - `make test` ✅ - Black-box binary check: - `go build -o /tmp/asc .` - Captured output artifact: **`/opt/cursor/artifacts/asc_bypass_keychain_semantics.txt`** - Shows: - unset + empty => `Credential storage: System Keychain` - truthy `yes` => `Credential storage: Config File` + bypass warning with accepted truthy values. <p><a href="https://github.com/rudrankriyam/App-Store-Connect-CLI/pull/529"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/view-pr-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/view-pr-light.png"><img alt="View PR" width="86" height="28" src="https://cursor.com/assets/images/view-pr-light.png"></picture></a></p> <p><a href="https://cursor.com/background-agent?bcId=bc-af09e7be-3d77-4fab-9085-6b5537ce37ef"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img alt="Open in Cursor" width="131" height="28" src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> <a href="https://cursor.com/agents?id=bc-af09e7be-3d77-4fab-9085-6b5537ce37ef"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-web-light.png"><img alt="Open in Web" width="114" height="28" src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a></p>

kerem commented

2026-02-26 21:33:47 +03:00

Author

Owner

@rudrankriyam commented on GitHub (Feb 14, 2026):

Fixed in #529 (merged to main).

@rudrankriyam commented on GitHub (Feb 14, 2026): Fixed in #529 (merged to main).

kerem referenced this issue

2026-02-26 21:34:26 +03:00

[PR #104] [MERGED] App store review submissions #288

kerem referenced this issue

2026-02-26 21:34:35 +03:00