Add adr-author skill, ADR template, and log security issues

- Introduced the `adr-author` skill for structured ADR creation; updated session setup and shared skills to require pre-loading it.
- Added `SKILL.md` and `ADR_TEMPLATE.md` for ADR authoring workflow and documentation.
- Updated protocol decision log with entries for the new skill and its integration.
- Documented two critical JWT logging security issues in `LOGGING_ISSUES.md`.
- Minor: added a cleanup Bash command in `settings.local.json`.
This commit is contained in:
Loretta 2026-04-25 07:24:16 +02:00
parent affa85e5c5
commit 37559b6dc4
5 changed files with 293 additions and 2 deletions

File diff suppressed because one or more lines are too long

File diff suppressed because one or more lines are too long

170
.github/skills/adr-author/SKILL.md vendored Normal file

File diff suppressed because one or more lines are too long

View File

@ -0,0 +1,68 @@
# ADR NNNN: <imperative short title>
<!--
Template for Architecture Decision Records, per the `adr-author` skill.
Copy this file to `<active-repo>/docs/adr/NNNN-<slug>.md` and fill in.
Remove this HTML comment and any placeholder lines you don't use.
NNNN = zero-padded 4 digits, one-past-max in the target docs/adr/ folder.
<slug> = short kebab-case derived from the title.
-->
## Status
<!-- One of: Proposed | Accepted (YYYY-MM-DD) | Superseded by ADR-XXXX (YYYY-MM-DD) | Rejected (YYYY-MM-DD) -->
Proposed
## Context
<!--
Objective description of the situation.
What's the problem or opportunity? What's the timing constraint — why decide now?
What's in scope / out of scope?
No opinion or recommendation here — just the facts that frame the decision.
-->
## Decision
<!--
What we decided, in one sentence or one short paragraph.
Imperative tense: "Adopt X", "Use Y", "Migrate to Z".
This is the headline — someone skimming the file should understand the decision
without reading any other section.
-->
## Consequences
**Positive:**
- <what this enables / improves>
- <...>
**Negative:**
- <what this costs / breaks / constrains>
- <...>
**Follow-ups required:**
- <what needs to happen next to fully realize this decision typically a `_TODO.md` entry or a subsequent ADR>
- <...>
## Alternatives considered
<!--
Minimum 2 alternatives. If you only had 1 "option", it wasn't a decision —
it was an implementation path. Consider whether this file is the right artifact.
-->
- **<alternative name>** (rejected): <one-sentence reason>
- **<alternative name>** (rejected): <one-sentence reason>
## Related
<!-- Remove lines that don't apply. -->
- Supersedes: <ADR-XXXX, if applicable>
- Superseded by: <ADR-YYYY, if this ADR was later overturned>
- Related ADRs: <ADR-ZZZZ, ...>
- Related TODOs/Issues: <LOG-T-N, BIN-I-M, ...>
- External references: <URLs, RFCs, blog posts that informed this decision>

View File

@ -172,6 +172,56 @@ See `LOGGING_TODO.md#log-t-11`.
- Sibling gap: `../SIGNALR/SIGNALR_ISSUES.md#sig-i-7` (same server-side plugin, protocol-options adoption gap)
- Plugin doc drift: `Nop.Plugin.Misc.AIPlugin/docs/SIGNALR/README.md:22` still documents the pre-migration `new AcBinaryHubProtocol()` registration (actual code uses `.AddAcBinaryProtocol(opts => {...})`). Update needed.
## LOG-I-9: JWT signing key written to log (CRITICAL security)
**Severity:** 🛑 **Critical (security)** · **Status:** Open · **Area:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:192`
### Description
`GenerateAccessToken` writes the JWT signing key to the logger via `GlobalLogger.Detail`:
```csharp
GlobalLogger.Detail($"Key: {configuration["JWT:Key"]}");
```
The signing key is the cryptographic secret used to sign all JWT access tokens. Anyone with read access to the logs (file system, log aggregators, retention archives, operator dashboards, screenshots during incident response) can:
1. Forge arbitrary tokens for any user
2. Impersonate any role / claim
3. Bypass authentication entirely
Log storage is typically at a lower trust level than dedicated secret stores. This exposure path is a complete authentication bypass.
### Fix direction
Remove the `GlobalLogger.Detail($"Key: ...")` line entirely. The signing key must NEVER appear in any log output. If presence verification is needed for diagnostics, log only a hash prefix (e.g. `Key configured: SHA256[0..8] = abc12345`).
### Related
- `LOG-I-10` (sibling — access token also leaked, same file, same code path)
- `LOGGING/README.md` may need a new "Never log secrets" guideline section (separate `_TODO.md` candidate)
- **Discovery context**: this issue and `LOG-I-10` emerged during the user-bearer-token ADR context-gathering phase (2026-04-25 Copilot session, while reading `AcLoginServiceServer.cs`). If a dedicated AUTH topic-folder is created later (likely as part of the bearer token ADR follow-ups), these two entries are candidates for relocation to `AUTH_ISSUES.md` with `Status: SUPERSEDED by AUTH-I-N` cross-references.
## LOG-I-10: JWT access token written to log (CRITICAL security)
**Severity:** 🛑 **Critical (security)** · **Status:** Open · **Area:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:212`
### Description
After `GenerateAccessToken` produces a token, the token text itself is written to the logger:
```csharp
GlobalLogger.Detail($"AccesToken: {writtenToken}");
```
(Side note: `AccesToken` is a typo for `AccessToken` — fix while at the line.)
Access tokens are bearer credentials — anyone holding the token can authenticate as the user until token expiry (currently 6h per `GenerateAccessToken` config). Logged tokens leak via the same channels as `LOG-I-9` (live streams, retention archives, screenshots, shared logs during ops).
### Fix direction
Remove the `GlobalLogger.Detail($"AccesToken: ...")` line entirely. If issuance verification is needed, log only **metadata** — user ID, expiry timestamp, issuer — never the token itself.
### Related
- `LOG-I-9` (sibling — JWT signing key also leaked, same file)
- Same `LOGGING/README.md` "Never log secrets" guideline gap as LOG-I-9
- Same discovery-context note as LOG-I-9 (bearer token ADR session, candidate for AUTH topic relocation if/when that emerges)
## Evaluated review findings — NOT bugs (by-design)
Cross-session review (2026-04) flagged several items that are **intentional defense-in-depth / YAGNI** and should NOT be "fixed":