This is an automated email from the ASF dual-hosted git repository.
FreeAndNil pushed a commit to branch Feature/security-audit-hardening
in repository https://gitbox.apache.org/repos/asf/logging-log4net.git
The following commit(s) were added to
refs/heads/Feature/security-audit-hardening by this push:
new dc17e1ba document the conventions the security scan work relied on #309
dc17e1ba is described below
commit dc17e1ba381460a6fd0eb0ac0660922e89350e7a
Author: Jan Friedrich <[email protected]>
AuthorDate: Tue Aug 18 00:49:40 2026 +0200
document the conventions the security scan work relied on #309
---
AGENTS.md | 24 +++++++++++++++--
CLAUDE.md | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
2 files changed, 103 insertions(+), 13 deletions(-)
diff --git a/AGENTS.md b/AGENTS.md
index 97ef271b..d6f9eec5 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -10,7 +10,7 @@ This is mandatory: skipping steps leads to duplicate reports
and wasted time for
### Step 1: Read the security model
Fetch and read the project's security model before evaluating any finding:
-https://raw.githubusercontent.com/apache/logging-site/refs/heads/main-site-pro/src/site/antora/modules/ROOT/pages/_threat-model-common.adoc
+https://raw.githubusercontent.com/apache/logging-site/refs/heads/main/src/site/antora/modules/ROOT/pages/_threat-model-common.adoc
Use this to answer:
- Is this component/behavior in scope at all?
@@ -18,6 +18,21 @@ Use this to answer:
If the finding is out of scope per the model, **stop here** and inform the
researcher.
+Two rules from the model account for most of what gets reported wrongly. Read
the model itself
+rather than relying on this summary, but know them before you start:
+
+- **Configuration is operator-supplied and trusted**, including configuration
URIs and the types,
+ members and converters that configuration names. A finding that needs the
attacker to control
+ configuration, a watched configuration file, or the format string of a log
statement is out of
+ scope: that is application misuse. Protecting those resources, and
transmitting them over a
+ confidential channel, is a deployer responsibility the model states
explicitly.
+- **Content is untrusted and must never be rejected**, because rejecting it
would turn a malicious
+ value into a denial of service, so it has to be escaped or sanitized
instead. Structural
+ identifiers, such as logger names or the identifier fields of a structured
log message, are
+ trusted, and there the model allows *rejecting* a malformed value rather
than silently altering
+ it. Getting these two the wrong way round produces a plausible-looking
report that proposes
+ exactly the wrong fix.
+
### Step 2: Check previously disclosed vulnerabilities
Read the project's Vulnerability Disclosure Report to check for duplicates:
@@ -29,7 +44,7 @@ If it overlaps with a known issue, **stop here**, link to the
existing advisory
### Step 3: Read the Security FAQ
Read the Security FAQ before concluding anything is a vulnerability:
-https://raw.githubusercontent.com/apache/logging-site/refs/heads/main-site-pro/src/site/antora/modules/ROOT/pages/security/faq.adoc
+https://raw.githubusercontent.com/apache/logging-site/refs/heads/main/src/site/antora/modules/ROOT/pages/security/faq.adoc
The FAQ lists behaviors that are **intentional and not vulnerabilities**.
If the finding matches an FAQ entry, inform the researcher that it is a known
non-issue
@@ -52,6 +67,11 @@ Assess the finding:
## Report quality rules
+- **Only call something a vulnerability when it really is one.** Name the
adversary, then check that
+ capability against the model. If it needs a misconfiguration, a co-resident
local user, or
+ anything the model does not grant, it is a correctness bug, a reliability
defect or hardening, and
+ saying so is more useful than a severity. Do not inherit the framing of a
scanner report that
+ arrived with severities already attached.
- Never speculate about impact beyond what you can demonstrate.
- Reproduction steps must be minimal and self-contained.
- Do not include unrelated findings in the same report: one issue per report.
diff --git a/CLAUDE.md b/CLAUDE.md
index 94d7f463..1aa100c9 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -26,33 +26,33 @@ almost always be doing.
(238 of 244 files in `src/log4net`). Copy it verbatim into new files.
- File-scoped namespaces (`namespace log4net.Appender;`). Note `.editorconfig`
still says
`csharp_style_namespace_declarations = block_scoped:silent`, but 242 of 244
files are
- file-scoped — follow the code, not that setting.
+ file-scoped: follow the code, not that setting.
- `using` directives outside the namespace, in one contiguous block.
### Language usage
-- **Explicit types, not `var`** — all three `csharp_style_var_*` options are
`false`.
+- **Explicit types, not `var`**: all three `csharp_style_var_*` options are
`false`.
Write `StringWriter writer = new(...)`.
- Target-typed `new()` and collection expressions (`private static readonly
char[] _x = [',', ';'];`).
- Omit the type wherever the target is known — including `return new(…);` and
`=> new(…);`, where
+ Omit the type wherever the target is known, including `return new(…);` and
`=> new(…);`, where
the enclosing member's return type supplies it. It cannot be omitted when
the target type is an
interface or abstract class, as in `Func<ISmtpTransport> f = () => new
MailKitSmtpTransport();`.
-- Expression-bodied members whenever the body fits on one line — this includes
constructors
+- Expression-bodied members whenever the body fits on one line, including
constructors
(`resharper_constructor_or_destructor_body = expression_body`).
- Braces on `if`/`else` bodies even for a single statement.
- `LangVersion` is `latest`, and current C# features are welcome and in use:
primary
constructors (`csharp_style_prefer_primary_constructors = true`), the
`field` keyword in
property accessors, list patterns, `switch` expressions.
- **Wrap long string literals with a multi-line raw string (`"""`), never with
`+`
- concatenation.** This includes attribute arguments — see the
`[Obsolete(...)]` message on
+ concatenation.** This includes attribute arguments; see the
`[Obsolete(...)]` message on
`log4net.Appender.SmtpAppender`. Raw strings have no line-continuation, so
each source line
break really is a `\n` in the value, but that is fine here: compiler
diagnostics render those
newlines as spaces, so a wrapped message still reads as one sentence. Raw
strings are constant
- expressions, so they are legal in attributes, and the feature is purely
syntactic — it works on
+ expressions, so they are legal in attributes, and the feature is purely
syntactic, so it works on
`net462`/`netstandard2.0` too.
- Private fields are `_camelCase`. Private fields and helper methods are
commonly placed
*after* the public surface of the type rather than at the top.
-### Nullability — the big constraint
+### Nullability, the big constraint
- `Nullable` is enabled solution-wide with `WarningsAsErrors=nullable`: **any
nullability
warning is a build error**, so it cannot be deferred.
- `log4net` targets `net462;netstandard2.0`. **Neither reference assembly is
nullable-annotated**,
@@ -68,7 +68,7 @@ almost always be doing.
- Use the internal `log4net.Util.Log4NetAssert` extensions rather than
hand-rolled checks:
`EnsureNotNull()`, `EnsureNotNullOrEmpty()`, `EnsureIs<T>()`. They carry
`[CallerArgumentExpression(nameof(value))]`, so no argument name is passed
at the call site.
- This includes constructor and property assignments — write `_x =
x.EnsureNotNull();`,
+ This includes constructor and property assignments: write `_x =
x.EnsureNotNull();`,
not `_x = x ?? throw new ArgumentNullException(nameof(x));`.
- Appenders never let exceptions escape to the caller. The house pattern is
`catch (Exception e) when (!e.IsFatal()) { ErrorHandler.Error("...", e); }`.
@@ -83,13 +83,44 @@ almost always be doing.
requires linking `NotNullAttribute`, `ValidatedNotNullAttribute` and
`CallerArgumentExpressionAttribute`, or you get `CS0122`.
- Analyzers (`Microsoft.CodeAnalysis.NetAnalyzers`, `AnalysisLevel 8`,
`src/log4net.globalconfig`)
- run on every build. **The solution builds with 0 warnings — keep it that
way.**
+ run on every build. **The solution builds with 0 warnings, keep it that
way.**
+
+### Documentation comments
+- **Every public and protected member gets an XML doc comment**, in test code
as well as production
+ code: test methods, nested helper classes and hand-written fakes included.
+- Use `/// <inheritdoc/>` when the member implements an interface or overrides
a base member, and a
+ real `<summary>` for everything else. `Log4NetTransaction` in the AdoNet
test doubles is the
+ pattern to copy.
+- When checking whether a member is documented, remember that `[Test]`,
`#pragma` and
+ `// ReSharper disable` lines legitimately sit between the doc comment and
the declaration.
+
+### Writing, in code and everywhere else
+- **Never use an em dash (`—`) or en dash (`–`).** Use a plain hyphen, or
restructure with a colon,
+ comma or parentheses. This covers comments, XML docs, commit messages,
AsciiDoc and chat.
+- In AsciiDoc, ` -- ` is also forbidden: Asciidoctor renders a spaced double
hyphen as an em dash,
+ so it breaks the rule even though the source looks like plain hyphens. Grep
touched files for
+ `[—–]` and ` -- ` before presenting a change.
+- No underscores in identifiers, including test method names.
`AllContainsEveryFlag`, not
+ `All_ShouldContainAllFlags`. (Private fields are `_camelCase`, which is the
one exception.)
### Tests
- NUnit 4, not MSTest, and always the constraint model: `Assert.That(actual,
Is.EqualTo(expected))`
(810 uses of `Assert.That`, zero of `Assert.AreEqual`). `[TestFixture]`,
`[Test]`, `[TestCase]`,
with `[SetUp]`/`[TearDown]` for per-test state.
-- `NUnit.Analyzers` warnings are errors too — e.g. NUnit1032 requires an
`IDisposable` fixture
+- Use an expression body for a single-statement test: `public void X() =>
Assert.That(...);`.
+- **`log4net` has no `InternalsVisibleTo`**, so private and internal members
are exercised through
+ reflection, not by widening their accessibility. See `SystemInfoTest`,
`LevelMappingTest` and
+ `UserNameFixingTest` for the `BindingFlags.Static | BindingFlags.NonPublic`
pattern.
+ `log4net.Ext.Mail` does grant `InternalsVisibleTo` to its own test project.
+- Mark a test `[NonParallelizable]` when it mutates static state
(`LogLog.InternalDebugging`, a
+ static field on a test double, a process-wide native registration).
+- Wrap expected internal logging in
`LogLog.ExecuteWithoutEmittingInternalMessages(...)` and capture
+ it with `LogLog.LogReceivedAdapter` rather than letting it reach the
console. Appender errors are
+ emitted by default, so a test that provokes one will otherwise add noise to
the suite output.
+- Guard platform-specific tests with `[Platform("Win")]` /
`[Platform("Linux")]`. A test that only
+ runs on Windows leaves the behaviour unverified in local Linux runs, so
prefer a cross-platform
+ home for the assertion when one exists.
+- `NUnit.Analyzers` warnings are errors too: for example NUnit1032 requires an
`IDisposable` fixture
field to be disposed in a `[TearDown]` method.
- For code that talks to the outside world, introduce a narrow interface and
hand-write a fake;
there is no mocking library in any test project. See `ISmtpTransport` /
`FakeSmtpTransport`.
@@ -97,6 +128,45 @@ almost always be doing.
`dotnet test src/<project>.Tests/<project>.Tests.csproj`.
- **When inspecting build output, redirect it to a file and read the whole
thing; do not pipe
MSBuild through line-oriented tools.** `grep`/`Select-String` cannot match
across newlines, and
- MSBuild's console logger formats differently when piped than when redirected
— a multi-line
+ MSBuild's console logger formats differently when piped than when
redirected, so a multi-line
diagnostic message then looks truncated when it is not. Before reporting
that the toolchain
mangles something, re-check with `dotnet build … > out.txt 2>&1` and inspect
`out.txt`.
+
+## Changelog
+
+Every user-visible change gets an entry in `src/changelog/<unreleased
version>/`, named
+`<issue>-<kebab-case-slug>.xml`. The format is the log4j changelog schema:
+
+- `type` is one of `added`, `changed`, `fixed`, `removed`, `updated`.
+- **Every `<issue>` element requires both `id` and `link`**; the export fails
with
+ `missing attribute: link` otherwise, which is only caught by the Maven site
build.
+- Put anything that has no issue number, such as an external finding
identifier, in the description
+ text rather than inventing an `<issue>` for it.
+- `src/changelog/3.3.2/298-fix-interprocesslock-mutex-leak.xml` shows the
shape for a change that
+ came out of an external audit.
+
+## Documentation site
+
+The manual lives in `src/site/antora/modules/ROOT/pages/`. A new appender page
needs three edits,
+not one: the page itself, an `xref` line in `nav.adoc` (kept alphabetical),
and the appender table
+in `manual/configuration/appenders.adoc`.
+
+## Security findings
+
+**[AGENTS.md](AGENTS.md) decides whether something is in scope and whether it
is a vulnerability.**
+Read it before triaging a report, and describe a finding in commit messages
and changelog entries
+the way it comes out of that assessment: a correctness bug, a reliability
defect or hardening is
+none the worse for being called one.
+
+What that leaves for this file is where the answers live in the code:
+
+- When a report is likely to recur on a path the threat model already settles,
leave a short comment
+ at the site with a link to the model rather than changing the code.
`XmlConfigurator` and
+ `XmlHierarchyConfigurator` carry these for the configuration-is-trusted
paths, and
+ `SystemStringFormat` for the format string.
+- `LocalSyslogAppender.EscapeNulCharacters` and
`RemoteSyslogAppender.ValidateIdentity` are the two
+ sides of the content and structural-identifier rule: content is escaped and
never rejected, a
+ malformed identifier is reported rather than quietly repaired.
+- Deliberate secure-default choices belong in the changelog with their opt-out
named, so that an
+ upgrade surprise is searchable. See the entries for `SendTimeoutMillis`,
`MatchTimeoutMillis` and
+ `LockTimeoutMillis`.
