FITB extensions - #3054
Conversation
|
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 |
|
Reviewed at Security
|
|
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. |
|
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. |
|
Sorry I'm phone only for the week. I can't believe i missed adding that
necessary file and I'll get that force pushed Saturday if i can.
In terms of security, I think https or visible with source seems
reasonable. Ultimately it is going to be the author who has to worry about
things.
All code is executed inside an anonymous function to avoid name space
collision. There is no true sandbox of any type. Broken Javascript will
potentially block the exercise from fully rendering but shouldn't interfere
with the rest of the page.
Brian
Message ID: ***@***.***>
… |
|
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. |
|
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 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. |
…nd custom specification of parsers.
…ries, config objects, or custom parsers.
…atex substitutions.
…x) calling external JS library and using config-json
dc5e696 to
f2071db
Compare
|
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 It is true that Changed the template called on the path for the library to use |
|
Thanks, review coming. I would not have blocked for the "blocker", but I'm seeing a few other nits that should be addressed. Like 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. |
|
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 And the remote-library gate is real, which I checked rather than assumed: Also: you were right and I was wrong about the remaining JSON control characters. I asked for BlockerThe new exercise is schema-invalid: These are the only new dev-schema errors in the PR. Duplication worth collapsing1. The stderr severity loop re-implements machinery already in Relatedly, 2. Raised on 2026-07-22, still openTrailing whitespace after The unreachable location check. The point last round was that the DocumentationThe Smaller: the Guide says a Style
Smaller observations
CommitsSubjects want the Two things I checked that turned out fine, so as not to leave them hanging: the Claude Opus 5, acting as a review assistant for Rob Beezer |
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._configinternal 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 afitb/@parserattribute that supercedes thefitb@modeattribute 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
assetsusing@sourceor 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 forrswill be linked in a separate comment.