Repository navigation
Publish wiki How-To guides - #193
Conversation
vharseko
added
documentation
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: The guide is wired into the real build and its links were checked against what actually gets published.
build.ymlrunscheck-how-to-links.pyaftermvn verifyon the ubuntu/JDK 11 row against the preprocessed tree; this PR's run printsChecked 4 How-To xrefs in 14 pages; 0 errors.andChecked 11 local How-To images.- The four
xref:openam:how-to:how-to-add-authorization-and-protect-your-application-with-openam-and-openig-stack.adoclinks reach the site unchanged (AntoraMojo.convertXrefsToAntora), and the page and its…-openam-setupanchor exist in the site's openamhow-tomodule. - All 85
doc.openidentityplatform.org/openig/how-to/<page>#<id>destinations in the wiki patch exist as ids in the head's How-To pages.
issue (non-blocking): The Accept/Content-Type check returns 406 for Accept: */*, for a request with no Accept header and for any Accept list.
openig-doc/src/main/asciidoc/how-to/how-to-protect-web-services-with-openig.adoc:527
The condition compares the first Accept value, up to its first ;, with the response media type for exact equality. JUEL evaluates the missing-header case to null rather than failing, so the condition is still true. After this step, the page's own earlier curl -v -X GET http://localhost:8080/ (Accept: */*, lines 307-315) gets 406 instead of 200. Browsers and Accept: application/json, text/plain get 406 too. I evaluated the replacement below with OpenIG's Expression against a response of application/json;charset=UTF-8. It gives false (pass) for no Accept, */*, application/json, application/json; charset=UTF-8, application/*, application/json, text/plain and a browser list. It gives true (406) for application/xml and text/plain.
"condition" : "${request.headers['Accept'] != null and not request.headers['Accept'][0].contains('/*') and not request.headers['Accept'][0].contains(response.headers['Content-Type'][0].split(';')[0])}",It does not handle q=0 exclusions, and text/* also passes.
issue (non-blocking): The MQ_Kafka settings table names the key boostrap.server, but the handler reads bootstrap.servers.
openig-doc/src/main/asciidoc/how-to/how-to-integrate-openig-and-message-brokers.adoc:72
MQ_Kafka.java:148 and :212 read only bootstrap.servers, with .defaultTo("localhost:9092"). A route written from the table therefore connects to localhost:9092, and Kafka ignores the misspelled key. The JSON examples on lines 58, 165 and 277 use the correct key.
|`bootstrap.servers` |Comma-separated list of host and port pairs that are the addresses of the Kafka brokersissue (non-blocking): The wiki stub keeps the Cyrillic anchor as настройка-openig, but the live wiki anchor is настройка--openig.
openig-doc/wiki-migration/wiki-how-to.patch:4405, :4237, openig-doc/wiki-migration/README.md:69
The removed heading is ## Настройка␠␠OpenIG, with two spaces (patch:4237). GitHub turns each space into a hyphen, and the live page has id="user-content-настройка--openig". Once the patch is applied, links to that anchor land at the top of the stub. That contradicts README lines 25 and 69 and the PR description.
+<a id="настройка--openig"></a>issue (non-blocking): The ICAP stub heading loses the old anchor icap-denial-of-service-in---dicaprewritetrue-mode.
openig-doc/wiki-migration/wiki-how-to.patch:3369, :3350
The old heading has two spaces before -Dicap (patch:3350). The stub heading has one, so its slug becomes icap-denial-of-service-in--dicaprewritetrue-mode and existing links land at the top of the page. Add an explicit anchor before the stub heading, and regenerate the hunk header:
+<a id="icap-denial-of-service-in---dicaprewritetrue-mode"></a>
## ICAP Denial of Service in -Dicap.rewrite=true Modeissue (non-blocking): The checker misses [source#id,…] anchors, which fails CI falsely, and accepts [[id]] inside ++++ passthrough blocks, which passes falsely.
openig-doc/check-how-to-links.py:65, :27
Line 65 collects only ^\[#id. visible_text strips ////, ---- and .... blocks but not ++++ blocks. With the change below, the 15 tests stay green, a [source#yes,yaml] destination resolves, and an id that exists only inside ++++ is reported as a broken anchor.
return re.sub(r"^(/{4,}|-{4,}|\.{4,}|\+{4,})[ \t]*\n.*?^\1[ \t]*$", "",
...
ids += re.findall(r"^\[[^\]#,\s]*#([^\].,%]+)", destination_text, re.MULTILINE)issue (non-blocking): The EmbeddedKafka table says Kafka will not start without zookeper.port, but it also starts when zookeper.connect is set.
openig-doc/src/main/asciidoc/how-to/how-to-integrate-openig-and-message-brokers.adoc:259
EmbeddedKafka.java:45 returns early only when zookeper.port is empty and zookeper.connect is undefined. Line 59 then uses zookeper.connect for a remote ZooKeeper.
|`zookeper.port` |Port of the embedded ZooKeeper. Kafka does not start unless `zookeper.port` or `zookeper.connect` is set
|`zookeper.connect` |Address of an external ZooKeeper, used instead of the embedded oneissue (non-blocking): The captured /secure response {"name":null} contradicts the sample controller, which returns the key hello.
openig-doc/src/main/asciidoc/how-to/how-to-protect-web-services-with-openig.adoc:346, :61
Line 61 returns Collections.singletonMap("hello", …). The route at this step only proxies the request, and line 789 shows {"hello":"John Doe"}.
{"hello":null}question (non-blocking): Were the expired sample JWTs kept on purpose as part of the historical captures?
openig-doc/src/main/asciidoc/how-to/how-to-protect-web-services-with-openig.adoc:768, :645, :865, :893, :720
All four tokens carry "exp":1726239022, which is 2024-09-13 14:50:22 UTC, and jwt.groovy rejects expired tokens (line 720). A reader who pastes the line 768 or 865 command gets 401 where the guide shows 200 {"hello":"John Doe"}. The bad-role token gets 401 instead of 403. Either answer leaves this Minor. If you kept them on purpose, add a NOTE before line 768 saying the tokens have expired and must be re-signed with private_key.pem (line 639). If not, issue tokens without an exp claim, or with a distant one.
question (non-blocking): Should the checker reject module-qualified xrefs such as xref:reference:page.adoc#id[], leaving ../<module>/ as the only cross-module syntax?
openig-doc/check-how-to-links.py:47
Only ROOT:index.adoc and targets with two colons are skipped. reference:filters-conf.adoc#X is resolved as how-to/reference:filters-conf.adoc and fails CI with missing page, even though it is the form AntoraMojo itself produces. The current pages use ../reference/, so CI is green. If the rejection is deliberate, say so in the docstring and README, and pin it with a test that expects missing page. If not, resolve <module>:<page> against <source>/<module>/<page>.
suggestion (non-blocking): Four checker branches survive mutation because the 15 tests use only bare ids, local images and :imagesdir: ../.
openig-doc/test_check_how_to_links.py:125, openig-doc/check-how-to-links.py:63, :65, :70, :73
At this head, each of the following leaves all 15 tests green:
[^\].,%]→[^\]](line 65)[^\],]→[^\]](line 63)- the URL skip replaced by
if False:(line 73) else ""→else "x"(line 70)
With the tests below added, the head runs 19 tests OK, and each mutant fails exactly one of them.
def test_shorthand_role_and_option_are_not_part_of_the_id(self):
result = self.check_source("[#present.role%option]\n== Title\nxref:#present[Present]\n")
self.assertEqual(0, result.returncode, result.stdout + result.stderr)
def test_reftext_is_not_part_of_the_id(self):
result = self.check_source("[[present,Present text]]\n== Title\nxref:#present[Present]\n")
self.assertEqual(0, result.returncode, result.stdout + result.stderr)
def test_remote_and_data_images_are_skipped(self):
result = self.check_source("image::https://example.com/a.png[]\nimage:data:image/png;base64,AAAA[]\n")
self.assertEqual(0, result.returncode, result.stdout + result.stderr)
def test_image_without_imagesdir_resolves_next_to_the_page(self):
result = self.check_source("image::picture.svg[]\n", {"how-to/picture.svg": "<svg/>"})
self.assertEqual(0, result.returncode, result.stdout + result.stderr)suggestion (non-blocking): No test or CI step pins the 85 How-To ids that the deferred wiki patch links to.
openig-doc/wiki-migration/wiki-how-to.patch:1, openig-doc/check-how-to-links.py:42-67
The checker never reads the patch, and it checks an anchor only when a local xref targets it. CI checked one fragment, #TokenTransformationFilter. Renaming any of the 85 ids leaves CI green and breaks a wiki deep link. The test below passes at this head. With how-to-integrate-openig-and-message-brokers-introduction renamed, it fails and names that page and id. It needs import re. When the patch is removed, the 85 pairs should move into a fixture.
def test_wiki_patch_destinations_exist(self):
doc = Path(__file__).parent
links = set()
for line in (doc / "wiki-migration/wiki-how-to.patch").read_text(encoding="utf-8").splitlines():
if line.startswith("+"):
links.update(re.findall(
r"https://doc\.openidentityplatform\.org/openig/how-to/([\w-]+)#([\w-]+)", line))
self.assertEqual(85, len(links))
for page, anchor in sorted(links):
text = (doc / "src/main/asciidoc/how-to" / f"{page}.adoc").read_text(encoding="utf-8")
self.assertIsNotNone(re.search(rf"(?m)^\[(?:\[|#){re.escape(anchor)}[\],.%]", text), f"{page}#{anchor}")suggestion (non-blocking): The checker never sees <<id>> or <<page.adoc#id,text>> cross references, yet its docstring and the README say it validates local xrefs.
openig-doc/check-how-to-links.py:44, openig-doc/wiki-migration/README.md:84
Only the xref: macro is matched. A broken <<…>> link gives Checked 0 How-To xrefs … 0 errors. No page uses << today. convertXrefsToAntora also rewrites only xref: macros, so a cross-module <<../reference/…>> link would break on the site in any case. Either reject << in How-To pages or extract it, and state which form the checker covers.
suggestion (non-blocking): The jwt.groovy error responses declare application/json but send single-quoted, unescaped text.
openig-doc/src/main/asciidoc/how-to/how-to-protect-web-services-with-openig.adoc:744, :743
The 401 and 403 bodies (lines 816, 857 and 913) fail any JSON parser, and a quote in an exception message breaks the structure. groovy.json.JsonOutput is in groovy-all 2.4.21. I did not run the changed script. Update the three captures to match.
response.setEntity(groovy.json.JsonOutput.toJson([error: message]))nitpick (non-blocking): Three commands use curl -v GET … without -X, and the captures show curl: (6) Could not resolve host: GET.
openig-doc/src/main/asciidoc/how-to/how-to-protect-web-services-with-openig.adoc:768, :796, :865
Drop GET, or write -X GET, and remove the resolve-error lines from the captures (:769-771, :797-799).
nitpick (non-blocking): A truncated sentence and several typos came over from the wiki.
openig-doc/src/main/asciidoc/how-to/how-to-protect-web-services-with-openig.adoc:619, :24, :70, :235, :296, :349, :354, :418, :621, :651
Line 619 ends with "OpenIG verifies JWT Sign and enrich". Typos:
- "need to user" (line 24)
- "gihub" (line 70)
- "sample-sevice" (line 235)
- "samplse-service" (line 296)
- "works file" (line 349)
- "OSWAP recomendations" (line 354)
- "different form" (line 418)
- "unauthenticatead" (line 619)
- "Genereate" (line 621)
- "mail filter chain" (line 651)
|
@maximthomas Addressed the review in 0136e06.
Validation: JDK 11 Maven The PR description and immutable patch links have been updated. Wiki publication remains deferred. |
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: The round-1 feedback is addressed in code and backed by executable checks.
- The checker resolves
module:pagexrefs (check-how-to-links.py:60-62), rejects<<…>>(:45) and skips verbatim/passthrough blocks (visible_text,:25-28). HowToScriptsTestruns the page's Groovy listings in CI: openig-doc failsafe went from 11 to 47 tests on build-maven (ubuntu-latest, 11).- Both lost wiki anchors (
настройка--openig,icap-denial-of-service-in---dicaprewritetrue-mode) are back and pinned bysrc/test/resources/how-to-wiki-destinations.txt.
issue (non-blocking): accept.groovy returns 500 when the upstream Content-Type has an unknown charset, has no subtype, or is empty.
openig-doc/src/main/asciidoc/how-to/how-to-protect-web-services-with-openig.adoc:553-564, :612-613, :538
MediaType.parse(contentType.type) and contentType.charset (Charset.forName) run outside the per-range try/catch, and response.headers.get(ContentTypeHeader) throws MalformedHeaderException for an empty value. I ran the listing through the real ScriptableFilter behind the route's RuntimeExceptionFilter with Accept: */*. Each of these upstream Content-Types gave a 500: application/octet-stream; charset=binary, text/html; charset=x-unknown, text, and an empty value. The page says only that malformed ranges are ignored. With the change below all four pass through, and the existing cases are unchanged. HowToScriptsTest plus the three rows: 46/46 green; without the change the three rows fail.
import org.forgerock.http.header.MalformedHeaderException
...
def representation
try {
representation = MediaType.parse(contentType.type)
if (contentType.charset != null) {
representation = representation.withCharset(contentType.charset)
}
if (contentType.boundary != null) {
representation = representation.withParameter(
'boundary', contentType.boundary)
}
for (parameter in contentType.additionalParameters) {
representation = representation.withParameter(
parameter.key, parameter.value)
}
} catch (IllegalArgumentException ignored) {
// An unparseable media type or an unknown charset is left unchanged.
return true
}
...
return next.handle(context, request).then({ response ->
ContentTypeHeader contentType
try {
contentType = response.headers.get(ContentTypeHeader)
} catch (MalformedHeaderException ignored) {
return response
}
if (accepts(request.headers['Accept']?.values, contentType)) {
return response
}Pin: add { new String[] { "*/*" }, "text/html; charset=x-unknown", 200 }, { new String[] { "*/*" }, "text", 200 } and { new String[] { "*/*" }, "", 200 } to acceptHeaders(). At :538, say "leaves responses without a parseable Content-Type unchanged".
issue (non-blocking): LlmGuardRail forwards the prompt unchecked when the guard model returns a JSON error or an empty choices list.
openig-doc/src/main/asciidoc/how-to/prompt-injection-mitigation-in-ai-systems-using-api-gateway.adoc:437, :429
In Groovy, null.every{} and [].every{} are both true. So !llmResponse.choices.every{ … == "SAFE" } is false for {"error":{"message":"model … not found"}} (a wrong model name or a provider error) and for {"choices":[]}, and the request goes to the LLM unchecked. Measured with groovy 2.4.21: error body → forwarded, empty choices → forwarded, SAFE → forwarded, INJECTION → blocked. The change below forwards only an explicit SAFE. It blocks the error body, empty choices, and a choice without message.
if (!llmResponse.choices || !llmResponse.choices.every{ it.message?.content == "SAFE" }) {issue (non-blocking): Both prompt-injection error helpers send {'error' : '…'} (single quotes) as application/json.
openig-doc/src/main/asciidoc/how-to/prompt-injection-mitigation-in-ai-systems-using-api-gateway.adoc:321, :389, :519, :523
This is the jwt.groovy pattern that this round fixed with JsonOutput.toJson; here a JSON client cannot parse the 403 body. Both listings already import groovy.json.JsonOutput. The new body is 61 bytes.
response.setEntity(JsonOutput.toJson([error: message]))At :519 use < Content-Length: 61; at :523 use {"error":"request is not allowed, prompt injection detected"}.
issue (non-blocking): The HTTP-to-Kafka example cannot work as written: the route names kafka-consumer, the heap declares only kafka-producer, and that producer writes to incoming-messages while the page reads topic1.
openig-doc/src/main/asciidoc/how-to/how-to-integrate-openig-and-message-brokers.adoc:99, :45, :59, :109, :136
The route fails to load with "Object kafka-consumer not found in heap" (HeapImpl.java:420, resolved at build in RouteBuilder.java:192). Pointing it at kafka-producer alone still sends the message to incoming-messages, so the consumer at :136 prints nothing. Change :45 to "Add Apache Kafka producer handler…", :59 to "topic.produce": "topic1", and :99 to "handler": "kafka-producer". The 202 after the change was not run, because that needs a broker.
suggestion (non-blocking): No acceptHeaders row reaches the Accept script's tie-break branches (more parameters at equal rank, max quality at equal specificity).
openig-doc/src/test/java/org/forgerock/openig/doc/HowToScriptsTest.java:50; script how-to-protect-web-services-with-openig.adoc:596-603
Three mutants survive 36/36: dropping bestQuality.max(quality), replacing type == bestType && parameters > bestParameters with false, and changing max to min. The page promises "the most specific matching range takes precedence, including a q=0 exclusion". With these two rows each mutant fails one case.
{ new String[] { "application/json;q=0, application/json" }, "application/json", 200 },
{ new String[] { "application/json, application/json;charset=utf-8;q=0" },
"application/json;charset=UTF-8", 406 },suggestion (non-blocking): jwtErrorsAreValidJson runs only jwt.groovy's error helper; the token check itself is never executed.
openig-doc/src/test/java/org/forgerock/openig/doc/HowToScriptsTest.java:140-157; script how-to-protect-web-services-with-openig.adoc:787-817
The test calls invokeMethod("getErrorResponse", …) and never calls run(). Two mutants survive 36/36: deleting the expiry throw at :803, and answering a missing token with Status.OK at :816. The pin below is green on the head and fails one case for each mutant. It needs imports for java.security.KeyPair, KeyPairGenerator, Signature, java.util.Base64, Collections and Map. The role-check fragment (:925-934) is not a runnable listing and stays unpinned.
private Response runJwtScript(Request request, Map<String, String> issuers, Handler next) throws Exception {
String text = article();
int start = text.indexOf("[source,Groovy]\n----\n", text.indexOf("`jwt.groovy`:\n"));
start = text.indexOf("\n----\n", start) + "\n----\n".length();
String source = text.substring(start, text.indexOf("\n----", start));
CompilerConfiguration configuration = new CompilerConfiguration();
configuration.addCompilationCustomizers(new ImportCustomizer()
.addImports("org.forgerock.http.protocol.Response"));
groovy.lang.Binding binding = new groovy.lang.Binding();
binding.setVariable("request", request);
binding.setVariable("context", new RootContext());
binding.setVariable("next", next);
binding.setVariable("iss", issuers);
Object result = new GroovyShell(getClass().getClassLoader(), binding, configuration).parse(source).run();
return result instanceof Promise ? (Response) ((Promise<?, ?>) result).getOrThrow() : (Response) result;
}
private static String signedJwt(KeyPair keys, long expirationSeconds) throws Exception {
Base64.Encoder base64 = Base64.getUrlEncoder().withoutPadding();
String claims = "{\"iss\":\"test\",\"name\":\"alice\",\"exp\":" + expirationSeconds + "}";
String signingInput = base64.encodeToString("{\"alg\":\"RS256\",\"typ\":\"JWT\"}".getBytes(StandardCharsets.UTF_8))
+ "." + base64.encodeToString(claims.getBytes(StandardCharsets.UTF_8));
Signature signature = Signature.getInstance("SHA256withRSA");
signature.initSign(keys.getPrivate());
signature.update(signingInput.getBytes(StandardCharsets.US_ASCII));
return signingInput + "." + base64.encodeToString(signature.sign());
}
@DataProvider
public Object[][] jwtExpirations() {
long now = System.currentTimeMillis() / 1000;
return new Object[][] { { now + 600, 200 }, { now - 600, 401 } };
}
@Test(dataProvider = "jwtExpirations")
public void jwtScriptChecksExpiration(long expirationSeconds, int status) throws Exception {
KeyPairGenerator generator = KeyPairGenerator.getInstance("RSA");
generator.initialize(2048);
KeyPair keys = generator.generateKeyPair();
String pem = "-----BEGIN PUBLIC KEY-----\n"
+ Base64.getMimeEncoder(64, "\n".getBytes(StandardCharsets.US_ASCII))
.encodeToString(keys.getPublic().getEncoded())
+ "\n-----END PUBLIC KEY-----\n";
Request request = new Request();
request.getHeaders().put("Authorization", "Bearer " + signedJwt(keys, expirationSeconds));
Handler next = (context, incoming) -> Promises.newResultPromise(new Response(Status.OK));
Response response = runJwtScript(request, Collections.singletonMap("test", pem), next);
assertThat(response.getStatus().getCode()).isEqualTo(status);
}
@Test
public void jwtScriptRejectsMissingToken() throws Exception {
Handler next = (context, incoming) -> {
throw new AssertionError("next must not be called");
};
Response response = runJwtScript(new Request(), Collections.<String, String>emptyMap(), next);
assertThat(response.getStatus()).isEqualTo(Status.UNAUTHORIZED);
}suggestion (non-blocking): No acceptHeaders row puts a parameter after q, so the RFC 9110 reading the script implements is unpinned.
openig-doc/src/main/asciidoc/how-to/how-to-protect-web-services-with-openig.adoc:577-589
The script is right. Under RFC 9110 §12.5.1, level=1 in application/json;q=0.5;level=1 belongs to the range, so a bare application/json gets 406. A mutant with the RFC 7231 accept-ext reading (} else if (!qualitySeen) {) returns 200 and survives 36/36. This row kills it:
{ new String[] { "application/json;q=0.5;level=1" }, "application/json", 406 },suggestion (non-blocking): No acceptHeaders row pins the qvalue's leading digit.
openig-doc/src/main/asciidoc/how-to/how-to-protect-web-services-with-openig.adoc:565
q=.5 is not an RFC 9110 qvalue, and the script drops that range, which is correct. A qualityPattern mutant that makes the leading digit optional (0?(?:\.[0-9]{0,3})?) survives 36/36. This row kills it:
{ new String[] { "application/json;q=.5" }, "application/json", 406 },suggestion (non-blocking): test_deferred_patch_uses_original_wiki_destinations checks only links with a fragment, so no test covers the 30 fragment-less How-To links the patch adds.
openig-doc/test_check_how_to_links.py:223-232
These are the stub "has moved to" links, the Home.md entries and the How-To-Guides.md link. I changed one stub link to convert-openam-token-to-oidc-jwt and one Home link to openig-http-clnt, and the suite stayed green. The test below fails on exactly those two and passes on the head (30 tests OK).
def test_deferred_patch_links_existing_pages(self):
doc = Path(__file__).parent
patch = doc / "wiki-migration/wiki-how-to.patch"
if not patch.exists():
self.skipTest("Deferred wiki patch has been removed after publication")
pages = set()
for line in patch.read_text(encoding="utf-8").splitlines():
if line.startswith("+"):
pages.update(re.findall(
r"https://doc\.openidentityplatform\.org/openig/how-to/([\w-]+)", line))
missing = sorted(page for page in pages
if not (doc / f"src/main/asciidoc/how-to/{page}.adoc").is_file())
self.assertEqual([], missing)suggestion (non-blocking): The zookeper.connect row does not say that a set zookeper.port still starts the embedded ZooKeeper.
openig-doc/src/main/asciidoc/how-to/how-to-integrate-openig-and-message-brokers.adoc:260
EmbeddedKafka.java:50-55 starts the embedded server whenever zookeper.port is non-empty. :59 then gives Kafka the zookeper.connect address, so a reader who sets both gets an unused ZooKeeper bound to that port.
|`zookeper.connect` |Address of an external ZooKeeper that Kafka uses instead of `localhost:<zookeper.port>`. Leave `zookeper.port` unset with it, otherwise an unused embedded ZooKeeper still starts on that port
suggestion (non-blocking): The patched How-To-Guides.md does not link the How-To PDF that deploy.yml publishes to the wiki.
openig-doc/wiki-migration/wiki-how-to.patch:128
The sibling pages Gateway-Guide.md and Reference-Guide.md carry a ([pdf](…/asciidoc/pdf/…)) link, and deploy.yml copies target/asciidoc/pdf into the wiki. The file name below comes from README:49 and was not checked against a build.
+See the [OpenIG How-To Guides](https://doc.openidentityplatform.org/openig/how-to/index) ([pdf](https://raw.githubusercontent.com/wiki/OpenIdentityPlatform/OpenIG/asciidoc/pdf/HowTo_Guides.pdf)).
nitpick (non-blocking): The invalid-role curl command has a line break inside the single-quoted Authorization value.
openig-doc/src/main/asciidoc/how-to/how-to-protect-web-services-with-openig.adoc:966-967, :976-977
A copied command sends a bare LF in the header value. Join :966 and :967 into one line ending …' http://localhost:8080/secure, and drop one of the two > lines at :976-977.
|
@maximthomas Addressed all twelve round-2 points in 2d7796a.
Validation: JDK 11 Maven The PR description and immutable patch links have been updated. External Kafka/LLM environments were not deployed; wiki publication remains deferred. Ready for re-review. |
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: The round-2 sample fixes landed where the bugs were.
- The prompt-injection error helpers now build their bodies with
JsonOutput.toJson([error: message])(prompt-injection-mitigation-in-ai-systems-using-api-gateway.adoc:321,:389), and the guard forwards only whenllmResponse.choicesis non-empty and every choice isSAFE. accept.groovypassescharset=binary,charset=x-unknownand baretextupstream types through unchanged. RowsHowToScriptsTest.java:110-112pin this, and the realScriptableFilterreturns 200 for all three. The HTTP-to-Kafka example now routes tokafka-producer.
issue (non-blocking): accept.groovy still returns a 500 when an upstream Content-Type has no media type (;, an empty value) or has a parameter without a value (application/json; foo).
openig-doc/src/main/asciidoc/how-to/how-to-protect-web-services-with-openig.adoc:556, :565-568
CHF parses ; into a ContentTypeHeader with a null type, and an empty value gets the same result on the client's path. MediaType.parse(null) then throws a NullPointerException. For application/json; foo the parameter value is null, so withParameter throws an NPE as well. Neither exception is an IllegalArgumentException, so the catch at :568 misses both and the route answers 500 instead of the unchanged response that :538 promises. I measured this through the shipped client (client-apache-async 3.2.2, JDK 11 and 26): a raw upstream sending each of the three values gives 500 at this head and 200 with the change below. The gap comes from my round-2 suggestion, which caught only IllegalArgumentException. The change follows what the range loop already does at :578-580 and :585-587:
def representation
try {
if (contentType.type == null) {
throw new IllegalArgumentException('Missing media type')
}
representation = MediaType.parse(contentType.type)
if (contentType.charset != null) {
representation = representation.withCharset(contentType.charset)
}
if (contentType.boundary != null) {
representation = representation.withParameter(
'boundary', contentType.boundary)
}
for (parameter in contentType.additionalParameters) {
if (parameter.value == null) {
throw new IllegalArgumentException('Missing parameter value')
}
representation = representation.withParameter(
parameter.key, parameter.value)
}
} catch (IllegalArgumentException ignored) {
// An unparseable media type or an unknown charset is left unchanged.
return true
}Pin: add { new String[] { "*/*" }, ";", 200 }, and { new String[] { "*/*" }, "application/json; foo", 200 }, after HowToScriptsTest.java:113. Both rows are red at this head and green with the change. With them HowToScriptsTest runs 62/62 on JDK 11. I did not run JDK 17 or 21.
suggestion (non-blocking): The "" Content-Type row goes through Headers.put, a path the shipped client does not take. The row stays green even though an empty upstream Content-Type gets a 500.
openig-doc/src/test/java/org/forgerock/openig/doc/HowToScriptsTest.java:125, :113
put with an empty String yields a GenericHeader, so headers.get(ContentTypeHeader) throws MalformedHeaderException and the script's catch returns the response. The CHF Apache client copies upstream headers with Headers.add (AbstractHttpClient.createResponse). On that path the empty value becomes a ContentTypeHeader with a null type and hits the NPE above. Switched to add, the "" row fails at this head and passes with the change above. All other existing rows stay green: 62/62 with the fix on JDK 11.
if (contentType != null) {
response.getHeaders().add("Content-Type", contentType);
}Pin: with this line, the "", 200 case is red at this head.
|
@maximthomas Addressed both round-3 points in 0d238a3.
Validation: JDK 11 Maven |
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: This commit closes the round-3 Accept-sample items in the script and in the test that runs it.
accepts()in theaccept.groovylisting now turns a missing media type and a parameter without a value into theIllegalArgumentExceptionits catch already handles (how-to-protect-web-services-with-openig.adoc:556-558,:568-570).;, an empty value andapplication/json; foofrom an upstream now leave the response unchanged instead of failing the route with a 500.HowToScriptsTest.java:127sets Content-Type withHeaders.add, the same path CHF's client uses for upstream headers. All 43acceptHeadersrows pass at the head.
Adds OpenIG How-To Guides to the documentation landing page, OpenIG navigation and PDF mapping. The companion OpenIG documentation change has been merged and its generated `how-to` module has been deployed to this repository's `master` branch. Generated pages and images remain owned by the existing OpenIG publication workflow. Validation: a current-version Antora integration build with the generated OpenIG module succeeded before the rebase. All fourteen How-To pages, local article links, fragments and images were checked. No How-To diagnostics; the 29 diagnostics elsewhere matched the committed baseline exactly. Historical release sources were excluded from the integration build. After rebasing onto the published module, `git range-diff` confirmed that the commit content is unchanged, and `git diff --check` passed. Companion PR: OpenIdentityPlatform/OpenIG#193.
The wiki migration from #193 has been published to the OpenIG wiki. Remove the deferred patch and its application instructions, along with the two tests that only checked the patch. The fixture and tests protecting the 85 original section destinations remain. Validation: `python3 -m unittest discover -s openig-doc -p 'test_check_how_to_links.py'` (28 tests passed); `git diff --check`.
Publishes thirteen practical OpenIG wiki articles as a searchable How-To guide, with ten local screenshots and an English authorization-flow diagram. Registers the guide in the existing Antora/PDF build and adds local xref/image checks to the Linux/JDK 11 workflow.
The migrated articles retain code listings and article-prefixed section anchors for the combined PDF. Corrected examples include Kubernetes Ingress v1, the REST configuration mount and OpenAM property, the Bearer header typo, and invalid JSON in the WebSocket excerpt. Historical example versions and sample JWTs are retained; an explicit note explains that the JWTs have expired and must be replaced with freshly signed tokens using matching keys.
The web-services tutorial handles Accept media ranges, wildcards, media parameters, quality values and specificity using the existing HTTP/Guava parsers. It preserves upstream responses with a missing or unparseable Content-Type, including a missing media type or parameter value, or an unknown charset. JSON errors, curl commands and captured response keys agree with the displayed code. The prompt-injection examples serialize JSON errors correctly, and the LLM guard forwards a request only when a non-empty set of choices explicitly reports SAFE. The HTTP-to-Kafka route consistently references its producer and topic; the setting descriptions explain the external ZooKeeper option and what happens when both ZooKeeper options are set.
The checker supports relative and module-qualified
xref:targets, recognizes source-style block IDs, excludes verbatim/passthrough contents, and rejects<<...>>references with guidance to usexref:. The regression suite checks local page URLs in the deferred wiki patch. A persistent fixture protects all 85 wiki section destinations after the patch is removed.Deferred wiki patch
The wiki patch is included as a reviewable file:
The patch retains the original section destinations, including duplicate-heading suffixes, the Cyrillic OpenIG configuration anchor and the ICAP heading with repeated hyphens. It updates Home and How-To-Guides, adds the How-To PDF link, and leaves existing wiki images available. It is not applied by the build.
Publish and verify the generated documentation and companion site navigation before applying the wiki patch. Wiki publication requires separate review and approval. The source wiki revision is
dcf9f150cac85b6feb937f17056b1650b3a9178c; refresh the patch if the wiki has changed.Validation
verify: all twelve selected reactor projects succeed; 1,461 tests pass with no failures, errors or skips, including 62 executable documentation-example cases.External integration environments were not deployed. Historical-version PDF rebuilding requires a supporting published version of the shared converter and is outside this migration.
Companion site PR: OpenIdentityPlatform/doc.openidentityplatform.org#45.