Skip to content

fix(oauth): clarify 2FA screen and add show-password toggle - #62

Merged
mashkovd merged 1 commit into
mainfrom
fix/enable-access-password-screen
May 18, 2026
Merged

mashkovd merged 1 commit into
mainfrom
fix/enable-access-password-screen

Conversation

@mashkovd

Copy link
Copy Markdown
Member

Why

A new client got stuck on the enable_access two-step-verification screen. The copy under-emphasised that the field wants the Telegram cloud password, so users re-enter the 5-digit login code, get invalid password, and have no way to see what they typed.

Changes

  • Copy: bold Telegram cloud password and state explicitly it is not the 5-digit login code from the previous screen.
  • Show/hide-password toggle: an eye button reveals the typed value. A real toggle needs JS — CSS cannot flip an <input> between type=password/type=text — so the password screen now emits a single inline <script> authorized by a fresh per-request CSP nonce (script-src 'nonce-<random>'). External and injected scripts stay blocked; the other enable_access screens keep the strict no-script CSP unchanged.

Tests

internal/oauth/enable_access_page_test.go:

  • <script> nonce echoes the CSP header nonce; no unsafe-inline.
  • nonce is regenerated per request.
  • non-password screens still forbid scripts entirely.

go build / go vet / go test ./... all green.

Users were getting stuck on the enable_access two-step-verification
screen: the copy under-emphasised that the field wants the Telegram
cloud password, so people re-entered the 5-digit login code and saw
"invalid password" with no way to check what they typed.

- Bold "Telegram cloud password" and spell out that it is NOT the
  5-digit login code from the previous screen.
- Add a show/hide-password eye toggle. A real toggle needs JavaScript,
  so the password screen now emits one inline <script> authorized by a
  fresh per-request CSP nonce (script-src 'nonce-<random>'). The other
  enable_access screens keep the strict no-script CSP unchanged.
@mashkovd

Copy link
Copy Markdown
Member Author

@codex review

@claude

claude Bot commented May 18, 2026 •

Copy link
Copy Markdown

Claude finished @mashkovd's task in 1m 30s —— View job


Review

  • Gather context
  • Read changed files
  • Analyze diff
  • Post findings

P3 (2) · P2 (0) · P1 (0) — No P1/P2 findings, good to merge.

The CSP nonce approach is implemented correctly: crypto/rand supplies 128 bits of entropy, base64.RawURLEncoding keeps the value safe in both the HTTP header and the HTML attribute without escaping mismatches, and html/template prevents any template-injection into the script block. The nonce is generated inside renderEnablePassword and overwrites any caller-supplied value, so there is no path to a static or attacker-controlled nonce.

Findings:

  • internal/oauth/enable_access_page.go lines 3–5 (not in diff, but worth noting): the package-level comment still says "They need no JavaScript" — stale after this change.
  • internal/oauth/enable_access_page_test.go line 69: TestRenderEnable_NonPasswordScreensHaveNoScriptSrc covers only the phone screen; the code and error screens carry the same invariant. Inline suggestion posted.

rec := httptest.NewRecorder()
renderEnablePhone(rec, enablePhonePage{})
if csp := rec.Header().Get("Content-Security-Policy"); strings.Contains(csp, "script-src") {
t.Fatalf("phone screen must forbid scripts entirely, got CSP: %q", csp)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3 — Only the phone screen is exercised here; the code and error screens have the same invariant but are untested. Consider ranging over all three:

Suggested change
t.Fatalf("phone screen must forbid scripts entirely, got CSP: %q", csp)
func TestRenderEnable_NonPasswordScreensHaveNoScriptSrc(t *testing.T) {
cases := []struct {
name string
fn func(http.ResponseWriter)
}{
{"phone", func(w http.ResponseWriter) { renderEnablePhone(w, enablePhonePage{}) }},
{"code", func(w http.ResponseWriter) { renderEnableCode(w, enableCodePage{}) }},
{"error", func(w http.ResponseWriter) { renderEnableError(w, "oops") }},
}
for _, tc := range cases {
rec := httptest.NewRecorder()
tc.fn(rec)
if csp := rec.Header().Get("Content-Security-Policy"); strings.Contains(csp, "script-src") {
t.Errorf("%s screen must forbid scripts entirely, got CSP: %q", tc.name, csp)
}
}
}

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@mashkovd
mashkovd merged commit 421f8a4 into main May 18, 2026
5 checks passed
@mashkovd
mashkovd deleted the fix/enable-access-password-screen branch May 18, 2026 22:59
mashkovd pushed a commit that referenced this pull request May 18, 2026
PRs #60 and #62 add new MCP tools (revoke_telegram_session,
get_user_audit_log), so this is a minor release. release-please
inferred a patch bump because the commits were typed fix:.
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