Skip to content

Credentials are never ::add-mask::ed, so a password not sourced from secrets stays unmasked #149

Description

@eiserv

What

easySFTP never emits ::add-mask:: for the credentials it receives. internal/gha/gha.go has Infof, Warningf, Errorf, Group, SetOutput and AppendSummary, but no masking helper, and action.yml passes password, private-key, passphrase and the three proxy-* counterparts straight into the step environment.

Why it matters

The GitHub runner masks values that came from secrets.* automatically, and docs/security.md correctly tells users to keep credentials in secrets. That covers the documented path. It does not cover:

  • a password produced by an earlier step's output, or read from a matrix entry, an environment variable, or a vault action's output. None of those are in the runner's mask list.
  • a user who ignores the advice and inlines a literal.

In those cases any future log line, error message or third-party stack trace that happens to interpolate the value prints it in cleartext into a log that is world-readable on public repositories.

Today the exposure is theoretical: the code does not print credentials, and sanitizeLine (issue #105) already stops a filename from forging a workflow command. But masking is defence in depth against a future log line, and it is roughly three lines of code.

Suggested direction

Add gha.AddMask(value string) (::add-mask:: with the same escapeData treatment as the other commands) and call it once at the start of run() in cmd/easysftp/main.go for every non-empty credential in the loaded config: Password, Passphrase, and the proxy equivalents.

Two caveats worth deciding explicitly:

  • Masking the private key is more awkward: ::add-mask:: is line-oriented, so a PEM block needs each line masked separately, and masking the short base64 lines of a key can garble unrelated log output. Masking only the passphrase and the passwords is probably the right scope; the key material itself is never printed anywhere.
  • Masking an empty or whitespace-only value must be skipped, otherwise the runner masks large parts of the log.

Low priority. Filing it so the decision is recorded rather than left implicit.

Metadata

Metadata

Assignees

No one assigned

    Labels

    securitySecurity-relevant change or hardening

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions