-
Type:
Bug
-
Resolution: Unresolved
-
Priority:
Major
-
Component/s: audit-trail-plugin
-
None
-
Environment:Jenkins 2.568.1 (jenkins/jenkins:2.568.1-jdk21), Jetty 12.1.8 (ee9), Java 21
audit-trail 455.v2ee2f7a_da_b_25 (code unchanged in 456.v39d2fd1ed556 and master)
No other plugins needed; CSRF protection on
Summary
AuditTrailFilter.handle() passes the live HttpServletRequest to a single-thread executor, and logRequest() then calls request.getParameter("name") (for /createItem) on that executor thread. At the same time the request thread runs ItemGroupMixIn.createTopLevelItem, which calls req.getParameter("mode") and req.getParameter("name").
The Jetty 12 ee9 Request object is not safe for concurrent use. When both threads parse the parameters at the same moment, the request thread can cache an empty parameter set. getParameter("name") then returns null and Jenkins answers:
HTTP/1.1 400 Bad Request X-Error: Query parameter 'name' is required
although the query string contains name. In our tests 4–9% of createItem calls failed. The opposite outcome also happens: the audit thread loses the race and the audit record for the job creation has no job name.
The race only exists when the URL pattern is configured, i.e. when $JENKINS_HOME/audit-trail.xml exists (or the pattern is set by UI or configuration-as-code) and it matches createItem. The default pattern matches it.
Impact: any tool that creates jobs through the REST API fails at random. A deployment that creates a few dozen jobs per run fails in about half of the runs and has to be retried.
Environment
- Jenkins 2.568.1 (official image jenkins/jenkins:2.568.1-jdk21), Jetty 12.1.8 (ee9), Java 21
- audit-trail 455.v2ee2f7a_da_b_25. The code in 456.v39d2fd1ed556 and on master is unchanged in this part
- No other plugins needed. CSRF protection can stay on
Steps to reproduce
- Build the image:
FROM jenkins/jenkins:2.568.1-jdk21 RUN jenkins-plugin-cli --plugins audit-trail:455.v2ee2f7a_da_b_25 ENV JAVA_OPTS="-Djenkins.install.runSetupWizard=false" - Start it, put this file at $JENKINS_HOME/audit-trail.xml and restart Jenkins (this is the default pattern, no loggers needed):
<?xml version='1.1' encoding='UTF-8'?> <hudson.plugins.audit__trail.AuditTrailPlugin plugin="audit-trail"> <pattern>.*/(?:configSubmit|doDelete|postBuildResult|enable|disable|cancelQueue|stop|toggleLogKeep|doWipeOutWorkspace|createItem|createView|toggleOffline|cancelQuietDown|quietDown|restart|exit|safeExit)/?.*</pattern> <logBuildCause>true</logBuildCause> <loggers/> </hudson.plugins.audit__trail.AuditTrailPlugin>
- Create jobs in a loop and count the responses:
#!/usr/bin/env bash # Usage: ./repro.sh <jenkins-url> [count] set -euo pipefail J=${1:-http://localhost:8080}; N=${2:-200} CJ=$(mktemp) CRUMB=$(curl -s -c "$CJ" "$J/crumbIssuer/api/json" | python3 -c 'import json,sys; d=json.load(sys.stdin); print(d["crumbRequestField"] + ": " + d["crumb"])' 2>/dev/null || true) XML='<?xml version="1.1" encoding="UTF-8"?><project><builders/><publishers/><buildWrappers/></project>' for i in $(seq 1 "$N"); do enc=$(printf 'repro job %s' "$RANDOM$RANDOM" | sed 's/ /%20/g') curl -s -o /dev/null -D - -b "$CJ" ${CRUMB:+-H "$CRUMB"} \ -X POST -H 'Content-Type: text/xml; charset=UTF-8' --data-binary "$XML" \ "$J/createItem?name=$enc" | tr -d '\r' | awk '/^HTTP/{s=$2} /^X-Error/{e=substr($0,10)} END{print s, e}' done | sort | uniq -c rm -f "$CJ"
Expected
All 200 jobs are created.
Actual
| Setup | createItem calls | 400 "Query parameter 'name' is required" |
|---|---|---|
| no audit-trail.xml | 200 | 0 |
| audit-trail.xml with the default pattern, run 1 | 200 | 15 |
| audit-trail.xml with the default pattern, run 2 | 200 | 7 |
Additional controls on a Jenkins with about 1000 jobs:
| Setup | createItem calls | 400 |
|---|---|---|
| no audit-trail.xml (empty home, 450–1000 jobs, restarts, keep-alive, busy executors and a full queue) | 1050 | 0 |
| audit-trail.xml, default pattern | 400 | 33 |
| audit-trail.xml, createItem removed from the pattern | 200 | 0 |
Root cause
AuditTrailFilter:
public boolean handle(HttpServletRequest req, HttpServletResponse rsp) throws IOException, ServletException { User user = User.current(); executorService.submit(() -> logRequest(req, user)); // live request handed to another thread return false; } private void logRequest(HttpServletRequest request, User user) { String uri = getPathInfo(request); if (uriPattern != null && uriPattern.matcher(uri).matches()) { ... } else if (uri.contains("/createItem")) { extra = formatExtraInfoString(request.getParameter("name")); // runs on the executor thread }
Jetty 12.1.8 org.eclipse.jetty.ee9.nested.Request keeps the parsed parameters in plain fields (_queryParameters, _contentParameters, _parameters) without synchronisation:
- extractQueryParameters() assigns _queryParameters = new Fields(true) first and decodes the query string into it afterwards.
- getParameters() checks the fields. If _queryParameters is non-null with size 0, it caches _parameters = _contentParameters, which is empty for an XML body. That cached value is returned for every later getParameter call on that request.
When the executor thread is between those two steps and the request thread enters getParameters(), the request thread caches the empty set. ItemGroupMixIn.createTopLevelItem then reads name == null and throws Failure("Query parameter 'name' is required").
uriPattern is a static field that is only set from AuditTrailPlugin.readResolve() (loading audit-trail.xml) or setPattern(). Without a saved configuration it stays null and logRequest never touches the parameters, which is why a fresh Jenkins does not show the problem.
Debug evidence
We added a log line to Request.getParameters() (thread name, identity of the request object, state of the parameter fields, whether name was found). For each failing request, the request thread and the audit executor thread work on the same request object:
getParameters req=<A> thread=Jetty-29 query=size=0 content=size=0 cachedIs=content name=NULL (mode) getParameters req=<A> thread=Jetty-29 query=size=0 content=size=0 cachedIs=content name=NULL (name -> 400) getParameters req=<A> thread=pool-N-thread-1 query=size=1 content=size=0 cachedIs=query name=ok
Reverse case, the request succeeds but the audit thread sees no name:
getParameters req=<B> thread=pool-N-thread-1 query=size=0 content=size=0 cachedIs=content name=NULL getParameters req=<B> thread=Jetty-43 query=size=1 content=size=0 cachedIs=query name=ok getParameters req=<B> thread=Jetty-43 query=size=1 content=size=0 cachedIs=query name=ok
Related risks (not tested)
- The /queue/cancelItem branch calls request.getParameter("id") on the executor thread in the same way, so the same race very likely applies to it.
- The executor can run after the response is complete. Jetty recycles and reuses request objects, so getRemoteAddr(), getQueryString() and getParameter() on the executor thread may read a recycled request or the next request on the same connection.
Suggested fix
Read everything logRequest needs on the request thread and pass only values to the executor. Taking name and id from getQueryString() instead of getParameter() also avoids the filter triggering body parsing for form posts:
@Override public boolean handle(HttpServletRequest req, HttpServletResponse rsp) throws IOException, ServletException { String uri = getPathInfo(req); if (uriPattern == null || !uriPattern.matcher(uri).matches()) { LOGGER.log(Level.FINEST, "Skip audit for request {0}", uri); return false; } User user = User.current(); String remoteIP = req.getRemoteAddr(); String queryString = req.getQueryString(); executorService.submit(() -> logRequest(uri, queryString, remoteIP, user)); return false; }
with logRequest working only on these values and extracting name / id from queryString.
Workaround
Remove createItem (and cancelQueue if the queue API is used by tools) from the audit-trail URL pattern. In our tests this brought the failure rate back to 0.
- relates to
-
JENKINS-76427 The audit trail plugin logs show login by NA instead of username
-
- Closed
-