Skip to content

fix: address potential bugs found in code review#150

Merged
yitsushi merged 5 commits into
mainfrom
bit-of-refactor
May 15, 2026
Merged

fix: address potential bugs found in code review#150
yitsushi merged 5 commits into
mainfrom
bit-of-refactor

Conversation

@yitsushi

Copy link
Copy Markdown
Owner

Bug fixes:

  • import: pass empty namespace to AddNamespace so accounts from new namespaces are not mistakenly treated as conflicts, causing spurious overwrite prompts
  • set-length: validate ParseUint result and re-prompt on non-numeric input instead of silently storing length=0
  • cmd: propagate terminal read errors from all askFor* helpers to prevent infinite loops on EOF/pipe close (add_token, set_length, set_prefix, rename, instant)
  • generateCode: return error instead of printing to stdout so callers can surface failures rather than displaying an empty token
  • CheckPasswordConfirm: use crypto/subtle.ConstantTimeCompare to avoid timing side-channel

Refactoring for testability:

  • terminal: extract Terminal interface; New() returns *ConcreteTerminal
  • storage: add PasswordProvider interface + StaticPasswordProvider; FileBackend accepts provider via WithPasswordProvider option
  • cmd: extract execute*/askFor* functions with injected Terminal and Storage dependencies; add prepareStorage helper

Linter:

  • replace deprecated wsl with wsl_v5, gomodguard with gomodguard_v2
  • fix all noinlineerr and wsl_v5 violations; update quality-check action

Tests (overall coverage 22% → 63%):

  • internal/cmd: add_token, change_password, delete, dump, error types, generate, import, list, rename, set_length, set_prefix, update
  • internal/security/algo: SHA1/SHA256/SHA512/Default Hasher (0% → 100%)
  • internal/security: OTPError.Error (97% → 100%)
  • internal/storage: error types, SetPassword, acquirePassword branches

Bug fixes:
- import: pass empty namespace to AddNamespace so accounts from new
  namespaces are not mistakenly treated as conflicts, causing spurious
  overwrite prompts
- set-length: validate ParseUint result and re-prompt on non-numeric
  input instead of silently storing length=0
- cmd: propagate terminal read errors from all askFor* helpers to
  prevent infinite loops on EOF/pipe close (add_token, set_length,
  set_prefix, rename, instant)
- generateCode: return error instead of printing to stdout so callers
  can surface failures rather than displaying an empty token
- CheckPasswordConfirm: use crypto/subtle.ConstantTimeCompare to avoid
  timing side-channel

Refactoring for testability:
- terminal: extract Terminal interface; New() returns *ConcreteTerminal
- storage: add PasswordProvider interface + StaticPasswordProvider;
  FileBackend accepts provider via WithPasswordProvider option
- cmd: extract execute*/askFor* functions with injected Terminal and
  Storage dependencies; add prepareStorage helper

Linter:
- replace deprecated wsl with wsl_v5, gomodguard with gomodguard_v2
- fix all noinlineerr and wsl_v5 violations; update quality-check action

Tests (overall coverage 22% → 63%):
- internal/cmd: add_token, change_password, delete, dump, error types,
  generate, import, list, rename, set_length, set_prefix, update
- internal/security/algo: SHA1/SHA256/SHA512/Default Hasher (0% → 100%)
- internal/security: OTPError.Error (97% → 100%)
- internal/storage: error types, SetPassword, acquirePassword branches

Signed-off-by: Victoria Nadasdi <victoria@efertone.me>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses several CLI/storage correctness issues (password handling, token generation error propagation, input parsing) and refactors terminal/storage/cmd code to improve testability, adding substantial new unit test coverage and updating lint/CI configuration.

Changes:

  • Refactors command handlers to inject Terminal and Storage dependencies and adds PasswordProvider support in storage to remove terminal I/O from the backend.
  • Improves error handling in user-input helpers and token generation (propagating terminal read errors; returning errors from generateCode).
  • Adds/expands test suites across internal/cmd, internal/security, and internal/storage; updates golangci-lint configuration and CI workflow.

Reviewed changes

Copilot reviewed 41 out of 41 changed files in this pull request and generated no comments.

Show a summary per file
File Description
main.go Minor error-handling refactor for lint compliance.
internal/terminal/terminal.go Introduces Terminal interface and ConcreteTerminal implementation.
internal/storage/password.go Adds PasswordProvider abstraction and static provider for tests.
internal/storage/filebackend.go Injects password prompting via provider; refactors control flow for error handling.
internal/storage/filebackend_test.go Updates tests for password provider injection; adds new password acquisition tests.
internal/storage/error_test.go Adds coverage for storage error message formatting.
internal/security/check_password_confirm.go Switches to constant-time compare for password confirmation.
internal/security/error_test.go Adds coverage for security error message formatting.
internal/security/algo/algo_test.go Adds coverage for hasher constructors.
internal/cmd/*.go Refactors commands into execute*/askFor* helpers with dependency injection and improved error propagation.
internal/cmd/*_test.go Adds new unit tests for refactored command logic.
go.mod Updates declared Go version/tooling dependencies.
.golangci.yml Updates linter configuration (disabling deprecated linters and adding wsl_v5 settings).
.github/workflows/quality-check.yaml Updates CI Go version and golangci-lint action version.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

yitsushi added 4 commits May 16, 2026 00:07
The filippo.io/age library changed its wrong-password error message in
v1.3.1; update the TestInvalidPassword assertion to match.

Signed-off-by: Victoria Nadasdi <victoria@efertone.me>
- filippo.io/age v1.2.1 → v1.3.1
- filippo.io/hpke v0.4.0 (new transitive dep of age)
- github.com/urfave/cli/v2 v2.27.6 → v2.27.7
- golang.org/x/{crypto,mod,sync,sys,term,text,tools} updated

Signed-off-by: Victoria Nadasdi <victoria@efertone.me>
Signed-off-by: Victoria Nadasdi <victoria@efertone.me>
Downgrade go-header to v0.5.0 to match the API expected by
golangci-lint v2.12.2 (v1.0.0 introduced breaking changes).
Run golangci-lint fmt to fix gofumpt formatting in otp.go and
otp_test.go.

Signed-off-by: Victoria Nadasdi <victoria@efertone.me>
@yitsushi yitsushi merged commit d75bd36 into main May 15, 2026
8 of 9 checks passed
@yitsushi yitsushi deleted the bit-of-refactor branch May 15, 2026 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants