Skip to content

FITB extensions - #3054

Open
dbrianwalton wants to merge 8 commits into
PreTeXtBook:masterfrom
dbrianwalton:fitb-extensions
Open

FITB extensions#3054
dbrianwalton wants to merge 8 commits into
PreTeXtBook:masterfrom
dbrianwalton:fitb-extensions

Conversation

@dbrianwalton

Copy link
Copy Markdown
Contributor

The RS component for fitb had the ability to load dynamic js libraries, but this was never previously implemented in PreTeXt to support that possibility. This is an attempt to put that into practice, as well as to provide an option to define a v._config internal variable that an author can pass an arbitrary data structure defined by JSON to configure the problem (instead of writing cod assignments). Libraries can support new parsers for answers, so there is also support for a fitb/@parser attribute that supercedes the fitb@mode attribute defining the function that takes the submitted answer and parses the result to whatever structure the answer checker needs.

Individual libraries can be provided locally in the assets using @source or imported from an external source using @url. Note: The RS component requires a slight tweak to support local assets being imported before it actually works. The relevant PR for rs will be linked in a separate comment.

@dbrianwalton

Copy link
Copy Markdown
Contributor Author

See RunestoneInteractive/rs#1311

@rbeezer

rbeezer commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

I asked my assistant to review this, with an added emphasis on security (coming next). Sounds like it at least needs a discussion with @bnmnetp. I expect we do "dangerous" stuff already with interactive but maybe this situation is different. Just for openers, would it make a difference if we required any JS to be located in the external directory where its source was visible?

@rbeezer

rbeezer commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Reviewed at 49d597b6. This adds three author-facing capabilities to dynamic FITB — external JS imports (jsimports/jslibrary), a JSON config blob (config-jsonv._config), and custom answer parsers (fillin/@parser). The escaping is careful and the feature reads coherently; the notes below are the security surface first, then correctness, then small items.

Security

@url loads arbitrary external JavaScript into the reader's browser

The Guide text describes it plainly: "each included library is imported dynamically using the Javascript import command by the Runestone component." So <jslibrary url="https://…/lib.js"/> causes that code to execute in the reader's browser when the exercise opens. The schema types @url as bare text — no scheme restriction (http is accepted), no allowlist, and no Subresource Integrity. Dynamic import() cannot carry an integrity hash at all, so SRI is not simply a missing option here — it is unavailable for this loading mechanism.

The trust picture divides, and it is worth deciding deliberately rather than by omission:

  • Self-hosted output — the author is already the trust root. Inline author JavaScript already runs today via setupScript, postRenderScript, and jscmp. So @parser, config-json, and even a locally-imported library add nothing to that boundary.
  • A multi-tenant host such as Runestone Academy@url is different in kind from inline JavaScript. Inline code sits in the reviewed source; an external URL is unpinned, can change after review, can serve different payloads over time, and over http can be substituted in transit. It runs in the shared origin alongside a reader's authenticated session. A book author thereby gains the ability to run arbitrary, evolving, third-party code in other readers' authenticated sessions on that domain.

The hosting side of this is the linked rs change (#1311), but PreTeXt is the layer that exposes the capability, so the decision belongs here too. At a minimum, consider constraining @url to https and documenting the multi-tenant implication in the Guide; a same-origin/allowlist policy would be stronger if the platform can express one.

config-json escaping — sound, with two small gaps

escape-json-string handles \ " / \n \t \r, backslash-first, and the two-layer escape (once by the escaper, once as a value inside the dyn_vars JSON string) resolves correctly — a build shows v._config = JSON.parse("{ \"date\": … }"). Two gaps remain: it does not escape the other JSON control characters (\b, \f, and U+0000–U+001F generally) or the JavaScript line terminators U+2028/U+2029, and config-json is not checked as well-formed JSON at build time. Either produces a runtime JSON.parse failure in the browser rather than a build-time diagnostic. Low severity — author-controlled, and it breaks the exercise rather than the reader — but worth a note.

@parser is raw author JavaScript (line 387)

<xsl:value-of select="@parser"/> places the attribute value directly into the parser array as code (the example uses a bare parseMatrix). This is within the existing author-JS model, but there is no validation, so a malformed value yields silently broken JavaScript.

quote-string does not escape the import path

The jslibrary path is emitted through quote-string, which only wraps in ". A " in @url or @source breaks out of the array string. Unusual and author-controlled, but the file already has escape-json-string; using it here would be more robust and consistent with config-json.

Correctness

The sample exercise cannot run — its library is not committed

<jslibrary source="code/btm-matrix-library.js"/> is emitted as external/code/btm-matrix-library.js, but that file is not in the PR (git ls-tree/find both come up empty). So parseMatrix, randomMatrix, rrefMatrix, and testMatrixEqual are undefined and the dynamic import 404s — the new find-row-echelon exercise in the sample article does not function. The library needs committing under the example's external directory, or the example reworked to something self-contained.

@mode='math-formula'@mode='math' (line 389) is a bug fix bundled into a feature

The schema has only ever allowed mode values {"string"|"number"|"math"}, so the previous @mode='math-formula' branch was dead — mode="math" fell through to the identity parser function(val){ return val; } and never reached getParser(). The rename is a genuine correctness fix, but it is unrelated to the imports/config/parser feature and would read more cleanly as its own commit.

Minor

  • The new warning at line 204, PTX:WARNING Extension file for JS import not correctly formatted, is missing the : after WARNING (the convention is PTX:WARNING:), carries no exercise id, and fires from inside a variable so the import is silently skipped with no location. It is also unreachable: the schema requires exactly one of @source|@url, so the otherwise branch cannot occur for a valid document.
  • Trailing whitespace after <![CDATA[ in the new example.
  • Two consecutive blank lines follow the js-import template.
  • The derived .rng matches the .rnc.

One further note: this branch and #3047 both derive from the same point and both edit xsl/pretext-runestone-fitb.xsl, schema/pretext-dev.rnc, and schema/pretext-dev.rng in the same regions, so whichever merges second will need a rebase.

Claude Opus 4.8, acting as a review assistant for Rob Beezer

@bnmnetp

bnmnetp commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

As your assistant points out, the trust root is the author. Then, I think what it is saying is that because Runestone has the ability to rewrite a page as it is being served that is another consideration. Personally, I trust the guy running Runestone.

I think its suggestion to enforce https, and possibly ensure that the js for these new libraries is in a visible place would not be bad move. Although I still think it comes down to the fact that we have to trust the authors.

@rbeezer

rbeezer commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Thanks, @bnmnetp. I don't think about this too often, so not well-versed in all the ramifications. So interested these days to see what my assistant has to say. I guess things are a bit more controlled on Runestone, since that guy has to approve authors. Self-hosted, I guess an "author" could make an alluring book, like "Calculus in Three Days" and load it with malicious stuff, but that really isn't anything new.

@dbrianwalton - what do you think of having source visible and/or requiring delivery over https? I like the idea of having source for any extensions running with the source for the actual text. So it doesn't get lost with a fork or a change in maintainer. I really dislike how WW problems are being served from folks' individual servers. That was an end-run that caught me out.

@dbrianwalton

dbrianwalton commented Jul 24, 2026 via email

Copy link
Copy Markdown
Contributor Author

@dbrianwalton

Copy link
Copy Markdown
Contributor Author

Doing the more thorough thinking about the sample-article that I should have done earlier, I went to verify dynamic variable extractions still work and encountered an issue since the extraction is now being done inside Node rather than using playwright. I will need to think through how Node is going to access the extension scripts as well.

@dbrianwalton

Copy link
Copy Markdown
Contributor Author

When looking at the substitutions process, with heavy support from Claude (because I was not familiar with how Node imports libraries), we identified some possible collisions that needed to be resolved. The new commits address the missing script file and some revision of the sample-article example to match some updates to the schema related to the order of inclusion of scripts. The original had a library function name that matched the name of a local variable, so the library and example were modified to resolve this.

Node can only import using a format called "EMS" and can only deal with local files — no import via url — and does not have the sandbox that a browser has. Imported libraries that come from a URL need to be downloaded in the Python script to the working directory. Because there is no sandbox, a publisher variable needed to be added to track which external libraries have been vetted by the author and allowed to run under Node. If this is not set, an ERROR is triggered identifying which library needs to be cleared.

In addition, order of loading for libraries could be relevant, so we need to match the import process used by the Runestone Component. Claude also identified that the previous code for substitutions did not actually preserve information on whether the substitution was supposed to be math mode or plain text, so the substitution format was modified to track both styles. Old style substitution files should continue to work as missing #plain or #latex children to the #eval-subst element will default to just use the contents of #eval-subst.

I can combine the commits and force push to a fewer number of commits if that is preferred, but left them separate for now in case you want to preserve context of earlier analysis.

@dbrianwalton

Copy link
Copy Markdown
Contributor Author

Ready for another review.

Squashed multiple commits to distinct stages of the request. This now enforces extension javascript is either in the project assets or served via https://. In addition, the publisher file now needs to explicitly white-list the https:// served files to provide a caution to the author since these scripts will be run inside node locally on their computer when static substitutions are generated.

Updated escape-json-string template to deal with U+2028/U+2029. Claude told me that the other substitutions would be irrelevant as the parser would have rejected them as valid characters even before processing the file if they had been present.

It is true that parser has raw javascript text. The author will need to verify/debug independently. This is not something that can be verified in XSL.

Changed the template called on the path for the library to use escape-quote-string instead of just quote-string.

@rbeezer

rbeezer commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Thanks, review coming. I would not have blocked for the "blocker", but I'm seeing a few other nits that should be addressed. Like PTX:ADVICE.

Claude missed it, I just recently retired "visible-id" for "unique-id". Should be totally cosmetic, the former was an alias for the latter. So let's keep up with that.

Lots of little stuff - sorry about that.

The error severity stuff is also very new, sorry for that. (I'm moving faster these days!)

Focused, discrete commits is our preference, so perhaps later commits have a natural place if fixed into a previous commit on an interactive rebase. Neat and tidy is always preferable, but don't do anything that'll give you a mess to deal with.

@rbeezer

rbeezer commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Thanks — this is a substantially stronger round, and the security question got taken seriously rather than argued away.

Confirming what's resolved. The missing library is committed and the new exercise now runs end to end: building the sample article to LaTeX, its matrices appear in the output with zero unsubstituted [%= markers and zero error placeholders. escape-json-string handles U+2028/U+2029. The jslibrary path now goes through escape-quote-string. The PTX:WARNING colon is fixed, and the seed and @mode messages now use mode="visible-id" rather than reading the attribute. The .rnc and .rng files are faithful — regenerating from the literate source leaves the tree clean.

And the remote-library gate is real, which I checked rather than assumed: preflightImports classifies every specifier before anything is loaded, and processExercise returns failedExercise(...) ahead of any import when its exercise named a refused one, so refused code never executes. The details are well judged — a legacy array-shaped file approves nothing, matching is exact so a near-miss URL is refused rather than accepted, http:// is refused in a way approval cannot override, and refusal is per-exercise so one bad import costs one exercise instead of the document. The error messages print the exact publication-file XML to paste along with the exercises that asked for each library. This is a better design than the last round asked for.

Also: you were right and I was wrong about the remaining JSON control characters. I asked for \b, \f and the C0 range; XML 1.0's Char production excludes U+0000–U+0008, U+000B, U+000C and U+000E–U+001F outright, and the three that are legal — tab, LF, CR — were already handled. Nothing to do there.

Blocker

The new exercise is schema-invalid: <setup> comes before <statement>. In examples/sample-article/sample-article.xml, <exercise label="find-row-echelon"> opens with <setup>. The FITB pattern is StatementExercise, (Setup? & Evaluation & …), so the statement comes first and everything else interleaves after it. Both siblings, dynamic-fitb-simple-formula and function-decomposition, have it that way.

error: element "setup" not allowed yet; expected element … "statement" …
error: element "statement" not allowed here; expected element "answer",
       "evaluation", "hint" or "solution"

These are the only new dev-schema errors in the PR.

Duplication worth collapsing

1. The stderr severity loop re-implements machinery already in common.py. The new loop in pretext.py reads each line for PTX:ERROR/PTX:WARNING/PTX:ADVICE and picks a log method. common.py already holds the canonical form of this: a token-to-level table covering FATAL, BUG, ERROR, FALLBACK, WARNING, DEPRECATE, INFO and DEBUG, used when relaying stylesheet messages, which also prefixes each with * and reports a malformed token as a bug. The new copy knows three tokens of the eight, so a PTX:FATAL or PTX:BUG coming out of the script is logged at whatever severity the previous line left behind, and a malformed token passes silently. Reusing the existing table would be shorter and would not drift.

Relatedly, PTX:ADVICE is not one of PreTeXt's severity tokens. It needs either adding to the canonical set deliberately or replacing with an existing one.

2. quote-string where escape-quote-string was the fix. The last round's point was that quote-string only wraps in quotes and escapes nothing, so a " in a path breaks out of the JSON string. pretext-runestone-fitb.xsl was changed accordingly — but extract-dynamic.xsl, new in this same PR, uses plain quote-string for three values that all land in JSON: the allowlist @url, exercise_visible_id, and the import path. A stray quote or backslash there produces malformed JSON, and the failure surfaces as a parse error in the script rather than as anything pointing at the cause.

Raised on 2026-07-22, still open

Trailing whitespace after <![CDATA[ in the new example, sample-article.xml:10315. Whole diff has 7 such lines across 3 files: 4 in the committed substitutions file, which come from \\ line endings the new library emits and so will return on every regeneration unless the library stops producing them; 2 in dynamic_extract.mjs comments; and that one.

The unreachable location check. The point last round was that the otherwise branch cannot fire, because the schema requires exactly one of @source|@url. It is still present in mode="js-import-path", and this PR adds two more of the same kind in pretext-validation-plus.xsljslibrary-no-location and jslibrary-both-locations — for conditions the schema already guarantees. Validation-plus is for constraints the schema cannot express; these two it can and does. The remaining PTX:WARNING still carries no exercise id.

Documentation

The <dynamics> publisher option is missing from the publisher Guide. It is explained well in doc/guide/author/topics.xml, worked snippet included, but doc/guide/publisher/publication-file.xml — where every other publication-file option has its reference entry — is untouched, and neither place links the other. A new publisher option wants both halves.

Smaller: the Guide says a fillin "is required to have a @mode … or a @parser", but the schema makes both optional and this PR adds a PTX:FALLBACK for the missing case, so "required" overstates it.

Style

  • Two-space indentation where the surrounding file uses four: the two new templates in pretext-runestone-fitb.xsl, and the whole new exercise in the sample article.
  • Comment --> alignment. The block above match="jslibrary" in pretext-validation-plus.xsl is ragged across all five lines, and the new three-line comment on json-replacements has a short last line. House style aligns the --> one column past the longest line. The new blocks in pretext-assembly.xsl and publisher-variables.xsl are aligned correctly and are the ones to match.
  • assets versus external. One validation-plus message says "within the project's assets", another says "relative to the external directory", for the same thing; the Guide says "assets directory". The managed directory is external.
  • The jslibrary-insecure-url message ends with a line much longer than its siblings, which all wrap near 72 characters, and says "the html version" rather than HTML.
  • "an <config-json>" → "a"; "Other control characters replacements" → "character"; javascript lowercase in the Guide's @parser sentence where the rest of the file writes Javascript.
  • A garbled comment in the mjs: "Modules are cached by the specifier that appears in the JSON / is cached between runs."
  • Space before /> on the new <jslibrary … /> where the <fillin …/> two lines below has none.

Smaller observations

  • classifySpecifier's scheme regex requires two or more characters, so a single-letter scheme falls through to the project-local branch. Not a hole — it then fails to resolve — but RFC 3986 is [A-Za-z][A-Za-z0-9+.-]*:, and on Windows a C:\… path would classify as project-local rather than off-project.
  • withinProject compares resolved paths but does not resolve symlinks, so a link inside the copied external tree pointing outward would pass.
  • fetch has no timeout; an unresponsive approved host hangs the build silently.
  • dynamic-representation falls back to the element's text only when both latex and plain are absent. Your note describes the fallback as covering a missing #plain or #latex; with one present and the other requested the result is silently empty. The script always writes both, so this is about the stated contract rather than observed behavior.
  • Both new messages in pretext.py embed a literal PTX:ERROR: prefix. None of the other thirty log.error calls in that file do, because the logger adds it — these will print it twice.
  • copy_managed_directories copies the entire external tree, images and all, for every substitution run, to resolve one library path.

Commits

Subjects want the Topic: description form throughout: GUIDE: should be Guide:, and the escape-json-string and dynamic-substitutions commits carry no topic. Element and attribute names are conventionally double-quoted in a subject. Several end with a period, and (v1) reads as noise. That applies whether or not they end up squashed.

Two things I checked that turned out fine, so as not to leave them hanging: the b-latex test for @ansobj does agree with fillin-solution as its comment claims, and narrowing exercise_evals to test[@correct='yes'] is required rather than merely tidy — your own new example has <eval obj="ans"/> in non-correct feedback, and a reader's response can have no static value at all.

Claude Opus 5, acting as a review assistant for Rob Beezer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants