fix(saml): address PR #97 review follow-up notes#102
Open
blicksten wants to merge 61 commits intooisee:mainfrom
Open
fix(saml): address PR #97 review follow-up notes#102blicksten wants to merge 61 commits intooisee:mainfrom
blicksten wants to merge 61 commits intooisee:mainfrom
Conversation
… rules Sync from claude-team-config repo: 19 agents (excl. mcp-specialist), 4 skills (orchestrate, plan-feature, review-pr, audit-plan), and composed CLAUDE.md (base + vibing-steampunk overlay). Add gitignore for settings.local.json. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update agents with modelTier/crossValidation fields, add Agent-as-Tool and Artifact patterns to orchestrate skill. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- 7 agents updated with palModel field (GPT-5.2-Pro / GPT-5.1-Codex) - PAL tool guidance in agent prompts (consensus, thinkdeep, codereview, etc.) - CLAUDE.md recomposed from base + overlay Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add SourceSearch: HANA fulltext search via SRIS textsearch API (client, handler, feature probe, XML types, integration test) - Fix transport API: use correct Accept headers, skip sap-client/sap-language params, rewrite ListTransports to use hierarchical parser - Add SkipSAPParams option to HTTP transport for endpoints that reject extra query parameters Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- CLAUDE.md: add Connected MCP Servers Usage Guide (VSP-SC3, Playwright, Context7) - CLAUDE.md: add Specialized Agents section with 6 agents - CLAUDE.md: update project status (100 tools, 35 integration tests, SourceSearch) - README.md: add Specialized Agents quick reference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Mandatory CV Protocol in 7 agent prompts - CV-GATE markers in orchestrate pipelines - New /cross-validate skill for Claude vs OpenAI debate - Fixed stale model refs, added cross-validate to skill list Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts: # .gitignore # pkg/adt/features.go # pkg/adt/transport.go
- Project Status: add transportable edits, GetAbapHelp, namespace support, HTTP proxy, .vsp.json, multi-system config - Codebase Structure: add pkg/config/, help.go, config_cmd.go - Key Files: add help and config entries - Last Session Reference: document upstream merge and conflict resolution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pareVersions) 3 new MCP tools for ABAP object version history via ADT Atom feed API. Supports PROG, CLAS, INTF, FUNC, INCL, DDLS, BDEF, SRVD with correct URL patterns per type (CLAS/INTF use /includes/main/versions). 10 unit tests, 4 integration tests, verified on SC3 with /COCKPIT/CL_TOOLS. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
GetUserTransports with targets=true returned empty on sandbox systems where transports have target=DUM. Now falls back to ListTransports (flat ADT + SQL) when tree response is empty. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Tools: 126 expert / 89 focused (was 99/52 in README, 103/58 in CLAUDE.md) - Unit tests: 238, Integration tests: 56 - Added v2.26.1 release notes (Version History, SourceSearch) - Updated capability matrix (Debugging = Y, Version History = Y) - Updated roadmap with all completed features since v2.15.0 - Fixed stale numbers in README_TOOLS.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 5 new MCP tools for ADT-native refactoring and quick fixes: - RenameRefactoring (evaluate/preview/execute via ADT rename endpoint) - ExtractMethod (evaluate/preview/execute via ADT extractmethod endpoint) - GetQuickFixProposals (auto-fix suggestions at error position) - ApplyQuickFix (apply specific quick fix proposal) - ApplyATCQuickFix (ATC finding quick fix with details/apply steps) New files: pkg/adt/refactoring.go, pkg/adt/quickfix.go, internal/mcp/handlers_refactoring.go Tests: 33 new unit tests (19 refactoring + 14 quickfix) Tools: 129 total (94 focused, 129 expert) Docs: ADT gap analysis, deep research spike, implementation roadmap Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 3 new MCP tools for testing and quality analysis: - GetCodeCoverage (run tests with coverage, returns statement/branch/procedure stats) - GetSQLExplainPlan (SQL execution plan with operators, cost, rows — HANA only) - GetCheckRunResults (detailed check run findings with line numbers and summary) New files: pkg/adt/testing.go, internal/mcp/handlers_testing.go Tests: 13 new unit tests in pkg/adt/testing_test.go Tools: 132 total (97 focused, 132 expert) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add DDLX/DCLS object support to GetSource/WriteSource, plus 2 new CDS analysis tools: GetCDSImpactAnalysis (where-used for CDS views) and GetCDSElementInfo (field metadata with annotations). New files: cds_tools.go, cds_tools_test.go, handlers_cds.go 10 new unit tests, all 577 tests passing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…oadmap Add 3 DDIC dictionary read tools (GetSearchHelp, GetLockObject, GetTypeGroup) following existing GetTable/GetView pattern. Add AddObjectToTransport for explicit transport object management. New: ddic_test.go (8 tests). All 4 roadmap phases now complete: 14 new tools, 64 new unit tests total. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Covers 4 priority areas: Intelligence Layer (impact analysis, semantic search), Workflow Enhancement (DSL improvements, templates), CI/CD Integration (GitHub/GitLab Actions), AI Enhancement (autonomous RCA, test generation). Projected: 150+ tools, 92%+ ADT coverage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…atus Corrected focused tools count (97 not 99), unit tests (297 not 302+), server.go comment (138 not 132), transport tools (6 not 5). Added ADT Refactoring, Testing/Quality, CDS/RAP, DDIC to status table. Updated roadmap section to reference FUTURE-PLAN.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… trail Add AnalyzeSQLPerformance, GetImpactAnalysis, AnalyzeABAPCode, and CheckRegression tools. These compose existing ADT capabilities into higher-level analysis for AI-driven code review. Key features: - Two-pass ABAP statement assembler with stringState FSM (handles single-quote, backtick, and |...| string template literals) - 21-rule ABAP code analysis engine (performance, security, robustness, quality categories) - 4-layer impact analysis (static refs, transitive callers, dynamic patterns, config-driven calls with SQL sanitization) - SQL performance analysis with HANA explain plan + text-based fallback - Diff-based regression detection (signature changes, removed methods, interface changes, RAISING clause changes) - 46 new tests, 343 total passing (277 in pkg/adt) Audit trail: 4 rounds (pre-plan, plan, post-implementation, PAL cross-validation). All 14 initial findings + 3 PAL findings resolved. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…gs resolved Round 5 fixes (Code Reviewer + Architect + PAL GPT-5.2-Pro): - Fix CRITICAL: reCallFuncDyn false positive on static CALL FUNCTION 'LITERAL' - Fix HIGH: SELECT loop "sticking" — METHOD/FORM boundary reset + aggregate exclusion - Fix HIGH: extractPublicMethods/extractInterfaceDef slice upper instead of source - Fix HIGH: extractRaisingClauses cross-method match via [^.]*? boundary - Fix HIGH: dynamic_call_no_try TRY scope tracking to prevent false positives - Fix HIGH: 6+ inline regexp.MustCompile moved to package-level vars - Fix MEDIUM: RAISING regex supports namespaced exceptions /FOO/CX_BAR - Fix MEDIUM: select_endselect excludes single-row INTO @ patterns - Fix MEDIUM: hardcoded_credentials covers backtick and |...| strings - Fix MEDIUM: AnalyzeSQLPerformance + AnalyzeABAPCode + CheckRegression 30s timeouts - Fix MEDIUM: todo_fixme rule moved before continue on IsComment - Fix MEDIUM: "safe" risk level changed to "unknown" on degraded paths - Remove dead code: reNormalizeWS 345 total tests passing (279 in pkg/adt). 5 independent audit rounds complete. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 5: TOC, Mermaid diagrams, collapsible sections, emoji markers, callouts — per Documentation Quality standard in base/CLAUDE.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- RunATCCheckTransport: run ATC check on all ABAP source objects in a transport in a single job (CLAS, INTF, PROG, FUGR), with findings summary - CreateATCRunMulti: batch ATC run for multiple object URLs in one request - TransportObjectToADTURL: convert transport object to ADT URL - RunATCCheckObjects: high-level helper for ATC on a list of ADT URLs - --timeout / SAP_TIMEOUT: configurable HTTP request timeout (e.g. 120s, 5m) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Security & correctness fixes from double audit (3 specialists + Chief Architect): CRITICAL/HIGH (blocking): - fix: xmlEscape() on objectURLs in CreateATCRunMulti (XML injection via & decode) - fix: Content-Type application/vnd.sap.atc.run.parameters.v1+xml (was application/xml) - fix: url.PathEscape in TransportObjectToADTURL (was manual /→%2f only) MEDIUM (should fix): - fix: url.Values for variant/worklistId query params (was fmt.Sprintf injection risk) - fix: url.PathEscape for worklistID in GetATCWorklist path - fix: Accept header application/vnd.sap.atc.worklist.v1+xml (was missing vnd.sap.) - fix: handle json.MarshalIndent errors in all 3 ATC handlers (was silently ignored) - fix: --timeout=0 now correctly disables timeout (cobra default 60s, removed >0 guard) - feat: add DCLS, DDLS, BDEF to TransportObjectToADTURL (RAP/CDS object support) - fix: improve error message when --enable-transports not set - fix: update tool description to list all supported types + required flag Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add v2.27.0 What's New entry in README.md - Add SAP_TIMEOUT/--timeout to config tables (README.md, CLAUDE.md) - Add RunATCCheckTransport to ATC section in README_TOOLS.md (2→3 tools) - Update tool counts: 89→90 focused, 126→127 expert across all docs - Add RunATCCheckTransport to focused Dev tools list in README.md - Add Last Session Reference (2026-02-25) to CLAUDE.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- .gitignore: add logs/, *.log, .playwright-mcp/, RAG/, NUL, test artifacts - .env.example: SAP connection template (no credentials) - PROJECT_ANALYSIS.md: project architecture analysis (710 lines) - SETUP_SUMMARY.md: setup and configuration summary (335 lines) - vibing-steampunk.code-workspace: VS Code workspace config Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
SSE connections are long-lived so WriteTimeout is intentionally omitted. ReadHeaderTimeout (5s) guards against slowloris. IdleTimeout (120s) reclaims abandoned connections. Audit: L-02 from Phase 8 double audit 2026-03-15. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Binary was showing 19/45 (outdated). Actual counts from server.go: - focused mode: 104 essential tools (103 in whitelist + 1 always-on) - expert mode: 144 total tools Binary is gitignored — rebuild with: go build -o vsp.exe ./cmd/vsp/ Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Replace absolute vsp.exe paths with ./vsp.exe in SETUP_SUMMARY.md and PROJECT_ANALYSIS.md. Add .claude/.sync-manifest.json to .gitignore to prevent tracking of auto-generated files with personal paths. Part of Workspace Cleanup Phase 2. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
# Conflicts: # .gitignore # CLAUDE.md # README.md # cmd/vsp/main.go # internal/mcp/handlers_codeintel.go # internal/mcp/server.go # pkg/adt/workflows.go
After merging upstream/main, our 18 tools were present as code but not registered in the new modular tool system. Add registration methods and focused mode entries for all contributed tools: - Refactoring: RenameRefactoring, ExtractMethod, QuickFix (5 tools) - Version History: GetRevisions, GetRevisionSource, CompareVersions (3) - Testing: GetCodeCoverage, GetSQLExplainPlan, GetCheckRunResults (3) - CDS: GetCDSImpactAnalysis, GetCDSElementInfo (2) - Intelligence: AnalyzeSQLPerformance, GetImpactAnalysis, AnalyzeABAPCode, CheckRegression (4) - Transport: RunATCCheckTransport (1) Co-Authored-By: Porfiry
DDLX (CDS metadata extension) and DCLS (CDS access control) support in the unified GetSource/WriteSource was lost when taking upstream's workflows_source.go during merge. Re-add DDLX/DCLS cases to: - GetSource read switch - WriteSource type validation - WriteSource existence check (upsert) - WriteSource create path - WriteSource update path - Tool descriptions in handlers_source.go Co-Authored-By: Porfiry
Accept upstream deletions of rejected PR code (codeanalysis v1, refactoring v1, quickfix v1). Fix mcp-go v0.47 API migration (request.Params.Arguments → request.GetArguments()). Re-add SourceSearch types to xml.go. Valuable fork code (impact, regression, sqlperf) will be re-added as separate commits. Co-Authored-By: Porfiry
Re-add valuable fork code after upstream merge: - analysis_types.go: shared types (CodeFinding, SQLPlanNode, etc.) - sql_explain.go: GetSQLExplainPlan method (extracted from testing.go) - impact.go: multi-layer blast radius analysis (4 layers) - regression.go: breaking change detection (4 rules) - sqlperf.go: SQL performance analysis (text + HANA plan) - handlers_intelligence.go: MCP handlers (mcp-go v0.47 API) - tools_register_fork.go: fork tool registration hook Extension hook pattern: ONE line added to tools_register.go calling registerForkTools(). All fork code in separate files. Co-Authored-By: Porfiry
- .claude/ directory: team agents, skills, commands (fork-only) - docs/PLAN.md: upstream merge & fork modernization RFC - docs/TASKS.md: migration task breakdown with rollback points Co-Authored-By: Porfiry
Rewrite refactoring tools using correct ADT REST API from abap-adt-api reference (marcellourbani): - RenameRefactoring: POST /sap/bc/adt/refactorings?step=evaluate&rel=... - RenameExecute: 3-step flow (evaluate → execute) - GetQuickFixProposals: POST /sap/bc/adt/quickfixes/evaluation Fixes PR oisee#82 rejection: wrong URLs, routing params, and body format. 3 unit tests, safety checks (RenameExecute blocked in read-only mode). Co-Authored-By: Porfiry
Per upstream owner's request (PR oisee#86): rebuild code analysis on top of the oracle-verified pkg/abaplint foundation instead of regex. New abaplint rules (5): - SelectStarRule: detect SELECT * FROM - HardcodedCredentialsRule: detect password/secret in literals - CatchCxRootRule: detect overly broad CATCH cx_root/cx_sy_* - CommitInLoopRule: detect COMMIT WORK inside LOOP/DO/WHILE - DynamicCallNoTryRule: detect dynamic CALL without TRY/CATCH Fixed: - ColonMissingSpaceRule Row:0 bug (now uses correct 1-based line) Integration: - codeanalysis.go runs abaplint linter as second pass, merges findings - 26 total rules (21 regex + 5 AST-based) - 17 new abaplint tests + 9 new integration tests (all passing) Co-Authored-By: Porfiry
Remove the entire regex-based statement assembler and 21-rule engine. Replace with pure pkg/abaplint lexer+parser analysis (13 rules). This addresses PR oisee#86 feedback: "consider building on top of the existing pkg/abaplint lexer + statement parser" instead of the "parallel, less precise implementation". Changes: - codeanalysis.go: 894 → 210 lines (remove regex, keep abaplint only) - Sorted findings by (line, rule) for deterministic output - 10 clean tests (select_star, hardcoded_credentials, catch_cx_root, commit_in_loop, dynamic_call_no_try, obsolete_statement, clean_code, sort_order, too_large, rules_applied) Co-Authored-By: Porfiry
Replace hybrid regex+abaplint engine with pure abaplint analysis. Apply PAL code review fixes: SelectStarRule COUNT(*) false positive, PreferredCompareOperatorRule config, handler input validation, json error handling. Restore ddic_test.go and handlers_codeanalysis.go. Co-Authored-By: Porfiry
Comprehensive spike analyzing 4 options for SAML auth to SAP S/4HANA Public Cloud. Cross-validated [C+O] with GPT-5.2-Pro + GPT-5.1-Codex (8/10 confidence). Plan audited: 3 MEDIUM findings fixed, zero MEDIUM+. Phases: fix browser-auth (PR#1) -> programmatic SAML (PR#2) -> credential-cmd (PR#3) Co-Authored-By: Porfiry
F1 (HIGH): Add 401 re-auth wiring — ReauthFunc callback in Config, singleflight in Transport, http.go + config.go added to modified files. F2 (MEDIUM): Remove google/shlex dep, use strings.Fields() instead. F3 (MEDIUM): CredentialProvider callback replaces credential zeroing — no long-term retention, fresh read on each auth cycle. Audit round 2: APPROVE [C+O] — zero MEDIUM+ remaining. Co-Authored-By: Porfiry
…Cloud Phase SAML.1: Fix browser auth to work with SAML/IAS SSO flows. - Fix cookie URL filtering: query multiple SAP paths (/sap/, /sap/bc/, /sap/bc/adt/) via cookieURLsForSAP() to capture path-scoped cookies - Improve poll timing: 500ms interval with elapsed time tracking and smart verbose logging (on cookie count change or every 10th poll) - Add verbose SAML redirect logging via chromedp.ListenTarget for page navigation and HTTP redirect events (URLs sanitized — no query params logged to prevent SAMLRequest/SAMLResponse leakage) - Extract matchesSAPAuthCookie/matchesSAPWeakCookie testable helpers - Harden URL construction: build targetURL and cookie URLs from parsed url.URL structs to handle query/fragment edge cases correctly - Add sanitizeURLForLog to strip query params and fragments from URLs Tests: 33 new test cases (6 URL, 10 auth cookie, 5 weak cookie, 5 sanitizer, classification, empty jar, existing). 2 integration tests under //go:build integration (SAML redirect chain, delayed cookie poll). Co-Authored-By: Porfiry
…2 LOW) - M-01: Rewrite TestBrowserAuth_PollDetectsCookies — old test was logically broken (network.GetCookies reads CDP cookie jar, doesn't make HTTP requests, so requestCount-based delayed cookies never appeared). New test uses meta-refresh redirect to simulate delayed SAML cookie appearance via actual browser navigation. - L-01: Fix TestEmptyCookieJar — was iterating empty slice (no-op). Now tests 7 real unrelated cookie names against both matchers. - L-02: Remove thread-unsafe requestCount (eliminated with M-01 fix). Audit: /check saml.1 — Porfiry [Opus 4.6] + GPT-5.2-Pro [C+O] All findings fixed. Zero MEDIUM+ remaining. Co-Authored-By: Porfiry
Co-Authored-By: Porfiry
…Phase SAML.2) Implement --saml-auth flag for headless SAML authentication via SAP IAS, enabling CI/CD automation without browser or MFA dependency. New: saml_auth.go with 4-step SAML dance (GET target → parse IdP form → POST credentials → follow SAMLResponse chain), x/net/html tokenizer for form parsing, CredentialProvider callback with []byte zeroing, host validation before credential POST, HTTPS downgrade protection. Modified: config.go (ReauthFunc field), http.go (401 re-auth with cookiesMu stampede protection), server.go (ReauthFunc passthrough), main.go (--saml-auth/--saml-user/--saml-password flags, processSAMLAuth, resolveConfig SAML awareness). 14 unit tests covering full flow, wrong password, IAS unavailable, malformed SAML, redirect loop, verbose-no-secrets, re-auth on 401, concurrent stampede protection, form parsing edge cases. Cross-validated: PAL codereview [gpt-5.2-pro] PASS (2 HIGH + 4 MEDIUM found and fixed), PAL thinkdeep [gpt-5.2-pro] PASS (zero findings). Co-Authored-By: Porfiry
…viders (Phase SAML.3) Add RunCredentialCmd with argv-based exec (no shell), JSON parsing, 30s timeout, and zeroed output buffer. Wire into --saml-auth with priority: credential-cmd > env vars > flags. Also fix Windows file permission test guard in browser_auth_test.go and mark SAML.2 GATE complete in plan. Co-Authored-By: Porfiry
Add TestSAMLLogin_HostMismatch and TestSAMLLogin_HTTPDowngrade to cover the credential exfiltration and HTTP downgrade guards in saml_auth.go. Found by /finish audit (F-01 MEDIUM, lead-auditor [Sonnet 4.6]). Mark PLAN-saml.md COMPLETE and update TASKS-saml.md with final status. Co-Authored-By: Porfiry
- callReauthFunc: set lastReauth only after fetchCSRFToken succeeds, preventing cooldown skip when CSRF fetch fails (PAL codereview M-01) - SAMLLogin: add validateFormAction() with case-insensitive host comparison and default-port normalization to prevent SAMLResponse exfiltration via malicious form actions in steps 3-4 (PAL M-02) - Add TestSAMLLogin_FormChainHostMismatch covering the new guard Found by PAL codereview [gpt-5.2-pro] + precommit [gpt-5.1-codex]. Co-Authored-By: Porfiry
SAP S/4HANA Cloud may respond to GET /sap/bc/adt/ with a SAMLRequest auto-submit form (HTTP-POST binding) instead of HTTP 302 redirect. Step 1b detects this form (has SAMLRequest, no j_username) and follows it to reach the actual IAS login page. Verified on live K0B DEV system (my413862.s4hana.cloud.sap). Based on discovery by guzel.davletshina. Co-Authored-By: Porfiry
…pagation /finish audit round 3 findings (PAL codereview gpt-5.2-pro): - Block HTTPS→HTTP downgrade on 302/301/303 redirects in CheckRedirect - Normalize host comparison in Step 2 using canonicalHost (consistent with Steps 3-4) - Use cmd.Context() instead of context.Background() for Ctrl+C support Co-Authored-By: Porfiry
Replace real S/4HANA Cloud tenant URL with example.s4hana.cloud.sap and real system ID with generic placeholder in tests. Co-Authored-By: Porfiry
…orts, cache, CDS) Resolve 6 merge conflicts: - devtools.go: keep both net/url (ours) and os (upstream) imports - analysis_types.go: keep our SQL types (used by sqlperf/regression) - codeanalysis.go: adopt upstream robustness category for catch_cx_root - codeanalysis_test.go: align test with upstream robustness category - rules.go: adopt upstream broadExceptions map + expanded credential names - lint_test.go: adopt upstream CX_SY_ no-issue + broad class tests Note: pkg/cache Example_withSQLite fails (upstream, requires CGO). Co-Authored-By: Porfiry
Co-Authored-By: Porfiry
1. credential-cmd help: note spaces limitation, recommend wrapper scripts 2. saml_auth.go: document string(password) Go immutability limitation 3. browser_auth.go: document cookie file as secret-bearing in SaveCookiesToFile 4. main.go: warn users that saved cookie files contain session secrets 5. http.go: add 30s context timeout on reauth path to prevent indefinite mutex hold during slow/unresponsive IdP SAML dance Addresses all 4 non-blocking notes from oisee's review on PR oisee#97. Co-Authored-By: Porfiry
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses all 4 non-blocking notes from @oisee's review on PR #97:
--credential-cmdhelp text: Added note aboutstrings.Fieldsspaces limitation and wrapper script recommendationstring(password)documentation: Added explicit comment insaml_auth.godocumenting the Go immutability limitation —[]byteslices are zeroed via defer, but theurl.Values.Set()string copy persists until GCSaveCookiesToFilewarning: Added doc comment marking the output file as secret-bearing; added runtime warning in verbose output when cookies are saved (both browser-auth and saml-auth paths)context.WithTimeouton the reauth path inhttp.goso the mutex is not held indefinitely when the IdP is slow or unresponsiveFiles changed
cmd/vsp/main.go--credential-cmdhelp text + cookie save warningspkg/adt/saml_auth.gostring(password)immutability boundarypkg/adt/browser_auth.goSaveCookiesToFileas secret-bearingpkg/adt/http.gocontext.WithTimeouton reauth pathTest plan
go build ./...— cleango test ./pkg/adt/... ./cmd/vsp/...— all pass🤖 Generated with Claude Code