I feel quite uncomfortable about this use case; it's a bit of a mess in terms of reviewability of content before it gets published, which is a key part of the code reviewer's rôle.

I feel quite uncomfortable about this use case; it's a bit of a mess in terms of reviewability of content before it gets published, which is a key part of the code reviewer's rôle.

The content is reviewed in the repos the code actually belongs to. All that is happening here is we are copying and pasting though previously reviewed and committed code into a doc folder (via curl in a code reviewed script).

I'd argue actually bundling it in the repo as we do now is far more dangerous as it will likely be blind merged. Somebody could easily add malicious code with the current workflow so let's not do that please. It's also adding confusion to users who think that is code to be reviewed/that impacts the site (note how code search yields a confusing result for mw-ui-icon in the MobileFrontend storybook directory).

With the immediate proposal, yes. But the next obvious step is to also copy e.g. jQuery from a CDN…

Why would we do that when we have access to npm?

I'm not sure what's making you uncomfortable as I can fetch external resources using node already. Curl simply provides convenience and more transparency by not hding those dependencies inside a js file which is a good thing.

I chatted to James in the office. @sbassett we leave the decision to install CURL on all docs.wikimedia.org sites with you. If the security team is fine with allowing fetching of resources URL via CURL, James is happy+ to enable it, otherwise I will move this code into a node script that does the fetching (cc @Jdforrester-WMF)

+ not to be confused with excited/James agrees with this

@Jdlrobson The code question, the npm run-doc script, does not run on doc.wikimedia.org. That is a static web server only. The code runs in a docker container on a disposable Jenkins node, and the output is then copied to the static server via a proxy.

The issue you ran into is that our CI environment used to be more monolithic where utilities like curl are pre-installed because we use them in our build scripts (e.g. all MW jobs use curl to check response codes of SpecialBlankPage and load.php?startup), and for MW itself (php7-curl naturally requires curl).

However in the past two years the CI environment has become more modular with npm-rundoc jobs having a dedicated Docker image that contains basically just Node.js and npm. So we only need to update the whitelist of aptitude packages in ci-config:/node10-test/Dockerfile to also include curl as we already do for various other jobs.

@Jdforrester-WMF I agree review is generally important for published artefacts but imho less so for doc.wikimedia.org. It is no less arbitrary to install karma and instanbul from npm and have their generated HTML/CSS/JS files published without review to doc.wikimedia.org/foo/v1.2.3/ than to fetch a stylesheet from en.wikipedia.org/load.php?… and store a permanent copy of that that same doc.wm.o/foo/v1.2.3/ directory. If anything, using our prod urls for that is inherently more trust worthy.

The alternative is to write a 10-line Node.js script that uses require('http') to fetch it and store it, which would work already. The only issue here is that MobileFrontend currently uses a Bash script instead of a Node.js script and thus curl is found to be missing there. This definitely doesn't need Security review. It's limited to CI configuration, and does not add additional capabilities that we don't already use much more severely for other use cases :)

What Timo said, those containers do not have curl.

Looking at the MobileFrontend script in dev-scripts/storybook.sh, it fetches resources from mediawiki/core and mediawiki/skins/MinervaNeue. Which mean that generating documentation depends on those two repositories. Shouldn't we have the CI job to clone both those repositories as well? Then instead of fetching the resources, the script would just look them up on local disk. That seems a little bit more robust to me?

In T239246#5699176, @hashar wrote:

Looking at the MobileFrontend script in dev-scripts/storybook.sh, it fetches resources from mediawiki/core and mediawiki/skins/MinervaNeue. Which mean that generating documentation depends on those two repositories. Shouldn't we have the CI job to clone both those repositories as well? Then instead of fetching the resources, the script would just look them up on local disk.

If we can get away with a shallow clone, this might be a better option from a security standpoint, since git is already installed within the node10-test image and we obviously trust these repos. Otherwise, as @Krinkle implied (I think?) updating the apt modules to include curl for node10-test is probably a bit easier and no less secure than writing some new Node scripts which would essentially do the same thing.

I dont mind copying local files or curl, however this is likely going to be a common requirement of extensions and skins. Right now getting storybook up and running seems to be blocked on RelEng and I'd much prefer this process not to repeat itself when a storybook is needed (which it will be) for a new repo. Thus if local route is taken please make sure it's easy to configure :)

Thanks for your input @Krinkle and @hashar

The overview

For MobileFrontend, CI does npm run-script doc which runs:

• npm run jsdoc
• npm run build-storybook
  • build-storybook -o docs/ui
• npm run coverage

Material from ./docs get pushed to doc.wikimedia.org after the change has been merged.

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/550516/ proposes to drop the mediawiki and MinervaNeue material committed in MobileFrontend and instead curl resources externally.

Jon raised a point about defining dependencies (eg MobileFrontend requires MinervaNeue). It is not that easy to define for CI, but potentially we could come with some extra field in extension.json that CI could process to clone the repositories. The storybook would then have to refer to them, I guess one could assume they get cloned as:

• \ (mediawiki/core
• \skins\MinervaNeue
• \extensions\MobileFrontend

Which is probably the case for most people. That has the benefit of being able to change Mediawiki / MinervaNeue locally and have the change reflected immediately in the Storybook.

CI would simply clone the repository, process the dependencies and clone them. We already have support for that in Quibble which can clone repositories defined in extension.json via "requires".

Something like:

quibble \
    --skip-deps \
    --skip-install \
    --resolve-requires \
    --command 'npm --prefix extensions/MobileFrontend install' \
    --command 'npm --prefix extensions/MobileFrontend doc'

In CI we would replace extensions/MobileFrontend with $THING_SUBNAME.

Then in .storybook/styles.less rewrite the imports to be relative:

@import '../../../skins/MinervaNeue/resources/skins.minerva.content.styles/tablet/common.less'

I am not sure how less will behave with the other relative @imports. Hopefully they are relative to the file that is executing the import.

@hashar if you want to test the local file approach (no curl) https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/550516/ has been modified to support what that would look like.

In T239246#5715404, @Jdlrobson wrote:

I dont mind copying local files or curl, however this is likely going to be a common requirement of extensions and skins. Right now getting storybook up and running seems to be blocked on RelEng and I'd much prefer this process not to repeat itself when a storybook is needed (which it will be) for a new repo.

Agreed, which is why we should add curl to the image and enable you to use existing bash scripts. Most other images have curl already. Per previous comments, this isn't about enabling networking as we already grant this to node, npm, wget and others. It's literally just curl itself.

Such bash scripts actually used to work in the pasts, on the persistent Jenkins nodes. It was lost because a Docker image is empty by default and only contains what is declared for that specific job. We didn't think to declare this until now, as it wasn't used anywhere.

The MediaWiki-specific jobs already use curl, for example to assert that Special:BlankPage and Special:JavaScriptTest render 200 OK (before we start the test). To catch common problems with a more descriptive error message.

Change 556444 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[integration/config@master] Add 'curl' to node10-test and bump downstream images

https://gerrit.wikimedia.org/r/556444

In T239246#5693999, @Jdforrester-WMF wrote:

With the immediate proposal, yes. But the next obvious step is to also copy e.g. jQuery from a CDN…

In T239246#5694112, @Jdlrobson wrote:

Why would we do that when we have access to npm?

I think using the jQuery CDN would provide more meaningful security (with dozens fewer layers of indirection and opportunities for compromise), than npm. But, I digress...

In T239246#5693968, @Jdforrester-WMF wrote:

I feel quite uncomfortable about this use case; it's a bit of a mess in terms of reviewability of content before it gets published, which is a key part of the code reviewer's role.

I agree, and this is a good practice to adopt. However, this is judging the way curl is used in this one example. It's not judging the tool (curl) itself. The same could've been written in Node.js or Python, and then it would work without issue.

We don't review the JS and CSS code that we bring in via Composer for generating PHPUnit's coverage report. Or the hundreds of npm packages involved in running JSDoc or grunt-karma. Or the frontend microsite that Storybook and JSDuck generate. I'm not sure why first-party includes would be any different. In terms of security, this is a sailed ship. Our strategy around that for doc generation has long ago set a direction towards technical security, not social security. That is, we've accepted that we want quick iteration and prototyping in this area, and that for other reasons (distrust of Gerrit, Jenkins and Cloud VPS) that we would fully isolate doc.wm.o in the backend (by network and hardware) and in the frontend (through CSP). Therefore its content are effectively without need to review – from a security perspective.

That isn't to say they shouldn't be HMAC'ed, or checked in, or version-locked; for correctness and to avoid defacing of our doc site. But, that's better left to individuals to implement. I don't think we want RelEng & friends to be a bottleneck for how each repo generates their doc sub-sites.

Besides, even when we tick all the boxes (HMAC, checked-in, version-lock), we might still want to pull them in from external resources to confirm they are identical and/or when staging an update and run a diff against them for ease of review (e.g. like for T203694 and T223602).

Forcing such scripts to be written in Python or Node.js using the http or requests package, and disallowing use of curl or bash commands in package.json, seems a bit arbitrary in that case. That's in addition to the use cases of curl that don't involve external downloads. In the Jenkins jobs for MediaWiki (Quibble) we use cURL to assert that the locally installed web server works correctly (health check / pre-promote test; similar to what we do in production with Scap/Swagger).

Does that addresses your concerns?

In T239246#5733355, @Krinkle wrote:

Does that addresses your concerns?

Not really. My point is that in general, we shouldn't be encouraging remote fetching of anything except through well-known, managed/HMAC-capable channels (like composer and npm), and in this particular case, these files are already available locally on disk during the CI run. Adding features to support use cases we actively think are bad is a poor decision.

I'm not suggesting that RelEng be a bottle-neck; quite the reverse. https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/550516/5/ achieves the aim without any RelEng-side changes (except it currently fails in the epic way that Storybook always seems to, with no good error messages).

In T239246#5733419, @Jdforrester-WMF

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/550516/5/ achieves the aim without any RelEng-side changes […]

ERR! Module not found: Error: Can't resolve '../.storybook/mediawiki-skins-MinervaNeue/skinStyles/mobile.startup/Overlay.less' in '/src/stories'

I believe this fails currently because our standalone npm-test jobs, like our composer-test jobs, do not clone MediaWiki core or dependent extensions. We'd have to set that up and and maintain it. E.g. a MediaWiki-specific fork of the node10-test jobs and underlying Dockerfiles. One where we introduce (part of?) Zuul-cloner and/or Quibble to create a MW-like directory structure and yet without being a full MW install (so it would have maintenance scripts available, but not PHP or Apache deps to use them).

If I understand correctly, it would also mean Reading Web can't go back to their low-maintenance approach of using the enwiki/load.php?module=foo endpoint. Instead they'd have to continue copying in individual files from the mediawiki-core clone and reference these uncompiled LESS directly.

To move forward, I see a few options:

1. Port their script to Node, and within CI (package.json) pull in some arbitrary npm package for making http requests, (and add a checksum check while at it).
2. Keep in Bash, add a checksum check, and add cURL to the image to make this work in CI as well.
3. Fork and maintain variants of the node10-test Docker images and Jenkins job to use Zuul-cloner/Quibble to almost set up a working MW site. (Then include those raw files from the other Git repo directly, using ../../ hacks, and keep parity between the ResourceLoader and Storybook LESS parsers for this subset of LESS files.)
4. A fresh idea for a different approach?

I'm lost now. From my point of view the answer is simple - you either give me access to CURL or make sure the documentation generation environment has local copies of core and Minerva for MobileFrontend with the same directory layout e.g. skins/ extensions/ inside a core folder/

ERR! Module not found: Error: Can't resolve '../.storybook/mediawiki-skins-MinervaNeue/skinStyles/mobile.startup/Overlay.less' in '/src/stories'

That's a mistake on my part because I can't keep up with the decision to use CURL or not use CURL. The path is wrong and is now fixed. The CURL solution is tried and tested and the easiest thing here.

To move forward, I see a few options:

Please make a decision either way and unblock reading web. I can work with CURL or with a local directory structure. Right now I just want to get these storybooks published as it's blocking conversations with my designers.

Fine, I'll merge and deploy.

Change 556444 merged by jenkins-bot:
[integration/config@master] Add 'curl' to node10-test and bump downstream images

https://gerrit.wikimedia.org/r/556444

Change 556469 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[integration/config@master] jjb: Use the new version of node10-test/etc. with curl

https://gerrit.wikimedia.org/r/556469

Change 556469 merged by jenkins-bot:
[integration/config@master] jjb: Use the new version of node10-test/etc. with curl

https://gerrit.wikimedia.org/r/556469

For reference, locally I have used:

export ZUUL_URL=https://gerrit.wikimedia.org/r/
export ZUUL_PROJECT=mediawiki/extensions/MobileFrontend
export ZUUL_REF=refs/changes/16/550516/6
export ZUUL_BRANCH=master

# packages-source set to composer to avoid cloning mediawiki/vendor:x
quibble \
    --skip-deps \
    --packages-source=composer \
    --skip-install \
    --command 'npm --prefix extensions/MobileFrontend ci' \
    --command 'npm --prefix extensions/MobileFrontend run-script doc' \
    mediawiki/skins/MinervaNeue \
    && xdg-open src/extensions/MobileFrontend/docs/ui/index.html

But the build fails on a relative path:

70% building 219/272 modules 53 active /home/hashar/workspace/src/extensions/MobileFrontend/node_modules/@storybook/addon-actions/dist/index.js
ERR! => Failed to build the preview
ERR! Module not found: Error: Can't resolve '../../../skins/MinervaNeue/skinStyles/mobile.startup/search/SearchOverlay.less
      in '/home/hashar/workspace/src/extensions/MobileFrontend/stories/search'

(node:5660) UnhandledPromiseRejectionWarning: ModuleNotFoundError:
  Module not found: Error: Can't resolve '../../../skins/MinervaNeue/skinStyles/mobile.startup/search/SearchOverlay.less'
    in '/home/hashar/workspace/src/extensions/MobileFrontend/stories/search'
    at factory.create (/home/hashar/workspace/src/extensions/MobileFrontend/node_modules/@storybook/core/node_modules/webpack/lib/Compilation.js:888:10)
    at factory (/home/hashar/workspace/src/extensions/MobileFrontend/node_modules/@storybook/core/node_modules/webpack/lib/NormalModuleFactory.js:401:22)
    at resolver (/home/hashar/workspace/src/extensions/MobileFrontend/node_modules/@storybook/core/node_modules/webpack/lib/NormalModuleFactory.js:130:21)
    at asyncLib.parallel (/home/hashar/workspace/src/extensions/MobileFrontend/node_modules/@storybook/core/node_modules/webpack/lib/NormalModuleFactory.js:224:22)
    at /home/hashar/workspace/src/extensions/MobileFrontend/node_modules/neo-async/async.js:2830:7
    at /home/hashar/workspace/src/extensions/MobileFrontend/node_modules/neo-async/async.js:6877:13
    at normalResolver.resolve (/home/hashar/workspace/src/extensions/MobileFrontend/node_modules/@storybook/core/node_modules/webpack/lib/NormalModuleFactory.js:214:25)
    at doResolve (/home/hashar/workspace/src/extensions/MobileFrontend/node_modules/enhanced-resolve/lib/Resolver.js:184:12)
    at hook.callAsync (/home/hashar/workspace/src/extensions/MobileFrontend/node_modules/enhanced-resolve/lib/Resolver.js:238:5)
    at _fn0 (eval at create (/home/hashar/workspace/src/extensions/MobileFrontend/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:15:1)
    at resolver.doResolve (/home/hashar/workspace/src/extensions/MobileFrontend/node_modules/enhanced-resolve/lib/UnsafeCachePlugin.js:37:5)
    at hook.callAsync (/home/hashar/workspace/src/extensions/MobileFrontend/node_modules/enhanced-resolve/lib/Resolver.js:238:5)
    at _fn0 (eval at create (/home/hashar/workspace/src/extensions/MobileFrontend/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:15:1)
    at hook.callAsync (/home/hashar/workspace/src/extensions/MobileFrontend/node_modules/enhanced-resolve/lib/Resolver.js:238:5)
    at _fn0 (eval at create (/home/hashar/workspace/src/extensions/MobileFrontend/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:27:1)
    at resolver.doResolve (/home/hashar/workspace/src/extensions/MobileFrontend/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:42:38)

A grep gives me:

stories/search/SearchOverlay.stories.js:import '../../../../skins/MinervaNeue/skinStyles/mobile.startup/search/SearchOverlay.less';
stories/search/searchHeader.stories.js:import '../../../skins/MinervaNeue/skinStyles/mobile.startup/search/SearchOverlay.less';

So stories/search/searchHeader.stories.js lacks an extra ../.

I have fixed src/extensions/MobileFrontend/stories/search/searchHeader.stories.js. Ran again the magic command this time passing --skip-zuul to prevent git to fetch and checkout over my local modification.

This time the build seems to work.

xdg-open src/extensions/MobileFrontend/docs/ui/index.html opens the web browser but the stories do not show up for some mysterious reason.