During a routine review of the startup manifest size (Grafana) I noticed that the WebAuthn extension registers 6 modules which is 5 more than what I would assume it needs given it has only one UI component (during auth/login flow), and its no where near the size to justify splitting as a form of payload optimisation.

Also per our perf guidelines (RL intro, frontend, backend) extensions with more than 3 modules should generally ask for help as it generally indicates a misunderstanding over how modules work in MediaWiki.

Change 607891 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/WebAuthn@master] Merge unused 'ext.webauthn.util' into 'ext.webauthn.ui.base'

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

Change 607891 merged by jenkins-bot:
[mediawiki/extensions/WebAuthn@master] Merge unused 'ext.webauthn.util' into 'ext.webauthn.ui.base'

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

Removing Security-Team tag as Platform Engineering are the ostensible owners of the code from our perspective, as they've recently spent both in-house and contractor engineering hours on this.

OK, we'll keep an eye out. I think we should probably be marked as stewards of Webauthn and Oathauth, but we need to discuss internally.

Thanks, I'll pencil in the rest of this review for this/next quarter.

Suggesting for a future quarter instead of ad-hoc so that we also plan and agree with PE in advance, so that we can then collab to make any adjustments as needed based on it and then develop/write those together. I don't know who took on tech lead/oversight on the Hello Welt contract for WebAuthn, but this might also be a good knowledge sharing oppertunity, either if there isn't someone currently on the team familiar with it , or more generally if you want additional/other people to be able to diagnose and maintain code of this extension in the future.

I tested WebAuthn on the Beta Cluster in Firefox, Safari and Chrome.

• "register" feature, by enabling on Special:Manage_Two-factor_authentication.
• "manage" features, by removing a key from Special:Manage_Two-factor_authentication.
• "login" feature, after enabling, log out and log in.
• "disable" feature, by disabling from Special:Manage_Two-factor_authentication.

Frontend structure

The JavaScript code uses a non-standard mw.ext. namespace for its code. This is not needed, not documented, and is fragile due to lacking an owner for the namespace. Thus there is risk of the namespace being replaced at run-time by another module, and thus the classes becoming undefined. (Perhaps this came from from reading unrelated documentation about Scribunto Lua modules where something called mw.ext does exist?)

The code should be defined under mw.webauthn instead of mw.ext.webauthn. Or, if the code is not in need of being called externally by other extensions, then perhaps a better approach would be to use the standard CJS approach of exporting and importing functionality, without it needing an assigned name through a global variable. More about that under Package files.

The files appear to have inconsistent names as well, which makes it difficult to navigate around the code base. For example, when enabling WebAuthn, the first code we interact with is register.js which references RegisterFormWidget but copying that to the text editor's quick open dialog, there is file matching that name in this project. Instead, navigating the ui/ directory we find a file with a shorter name, RegisterForm where this is defined as the only class in that file. Classes should generally be in a file that at least contains the last segment of the class name in full. In this case mw.webauthn.RegisterFormWidget should be found when searching for files matching "RegisterFormWidget".

The extension currently registers 5 distinct resource bundles. Given the small size of the code base, and the fairly standalone nature of its frontend, I believe this can and should be served as a single well-cached bundle. The module splitting applied is premature and costing more than it benefits. In particular it adds cost to unrelated page views through the startup manifest, but also negatively affects the WebAuthn performance through missed cache oppertunities by not having the feature preloaded in one go and then always used fast and local, instead of having 5 different scenarios where some part may have to come from another network request.

When combining the modules, the new index.js file will need to decide which HTMLForm to look for and hydrate. My recommendation would be to inject $form in the constructors from the index file instead of hardcoding in the Widget classes. You can then use a conditional check on the element existing, combined with wgCanonicalSpecialPageName === OATHManage to decide whcih mode to initialise. For example:

if ( mw.config.get( 'wgCanonicalSpecialPageName' ) === 'OATHManage' ) {

    var $form = $( '#webauthn-add-key-form' );
    if ( $form.length ) {
        var form = new mw.ext.webauthn.RegisterFormWidget( $form );
        ..
   }

}

Frontend lint

Various lint errors have been disabled that I'm fairly sure don't have a valid reason for being violated and disabled:

• The frontend code uses ES6 syntax. It is unclear if this is intentional or not. It's only const and let in a handful of places. The CI build is passing because the ESLint step has an invalid configuration file that causes this to be tolerated. It enables both "wikimedia/client-es5", "wikimedia/language/es6.json", which is not a valid combination. See eslint-config-wikimedia and ResourceLoader/ES6) for how to configure it as either an ES5 or ES6 project.

• The extension has a custom browser support policy documented on mediawiki.org but there does not appear to be a feature test or mention of this in the user interface. Registering it as ResourceLoader/ES6) will automatically help prevent a crash at run-time in older browsers. But it might also make sense to have some mention of browser support in the user interface. Right now it appears to leave users stranded.

• "no-prototype-builtins": "off". The code makes unsafe hasOwnProperty calls. These calls should done safely, instead of disabling the rule.

• "compat/compat": "warn". The code makes use of browser features that are not universally available in Grade A browsers as confgured in eslint-wikimedia compat. The extension currently has all compat validation disabled, which makes it difficult to have confidence in its code. The simplest approach is to not disable the compat rule and instead add the one new feature it uses (I believe Uint8Array) to eslintrc#globals, or override the compat/compat rule with the extension's own browser support policy.

Frontend size

Register/Enable on Special:Manage_Two-factor_authentication.

• Compressed: 73kB (HTTP ext.webauthn.manage|ext.webauthn.ui.base|oojs-ui||oojs-ui-core|oojs-ui-toolbars|oojs-ui-widgets|oojs-ui-windows)
• Uncompressed (mw.inspect)
  • ext.webauthn.ui.base 2.6 KiB
  • ext.webauthn.register 4.0 KiB

The code base only uses the following OOUI classes:

• OO.ui.IconWidget (oojs-ui-core)
• OO.ui.LabelWidget (oojs-ui-core)
• OO.ui.ButtonWidget (oojs-ui-core)
• OO.ui.HorizontalLayout (oojs-ui-core)
• OO.ui.ButtonInputWidget (oojs-ui-core)
• OO.ui.TextInputWidget (oojs-ui-core)

As such, the other OOUI dependencies can be removed which would bring the size down to 37kB.

Looking further, I notice that most of the workflows (including the Register/Enable workflow) follow the same pattern:

• Server-render a HTMLForm with only CSS from OOUI.
• JS then needlessly reproduces the same thing with client-side OOUI,
• there is one line of code that attaches an event handler to listens for the browser's WebAuthn API,
• upon success, submit the form, at which pont the server renders the next page.

As such, one additional step could be to attach that event handler directly to the element in question and thus no longer having to maintain any JavaScript code for the UI. This woudl bring the size down to under 2 KiB in total.

HTMLForm

The JS code makes unusual DOM queries such as find( 'span#button_add_key' ), 'input[name="remove_key"]', and find( '.removeButton' ) which are far too generic, fragile, and don't correspond with any of our coding conventions.

• Form input names are generally without any separator symbols, and in camelCase if multiple word are needed.
• Class names and IDs must be namespaced (e.g. mw-webauthn-), with camelCase for augmented words, and dashes for separating logical segments.

Misc

The extension has a very strange license: GPL-2.0-or-later AND GPL-3.0-or-later. I suspect this accidentally copied from the OATHAuth extension (which is the only other extension in existence with such license). That extension has this license because it contained multi-license source code however assumign all code in WebAuthn is original, this does not apply here. I assume we'd want to follow the MediaWiki default of GPL-2.0-or-later as already indicated by the file headers. Although, with many files missing a license header and code committed already, I don't know if we can just assume this. A confirmation from HelloWelt that they are okay with releasing this under GPL-2.0-or-later would help I guess. Someone more familiar with licensing may be able to make this decision without such confirmation.