This installs a series of safety nets:
* The selector [href*="#"] skips links without a fragment.
* It's still possible that a fragment exists, but is empty.
mwTitle.getFragment() checks this.
* The gateway does not assume the element exists, but checks this first.
If there is no such element, the gateway aborts the request in a way
that no error popup is shown. This is currently only possible with the
`{ textStatus: 'abort', xhr: { readyState: 0 } }` response as seen in
this patch. We might need to introduce a new, more clean way to silently
quit a fetchPreviewForTitle() call.
* The test for the reference gateway finally covers the scraping code.
Bug: T214970
Bug: T214971
Change-Id: I9ec57e0fbb0d21beaaa7b359c1c2bef64d2c14f5
Including tests for all situations.
I believe it is impossible or extremely hard to actually abuse any of
these places. All these data are not extracted from the current page, but
delivered either by MediaWiki's api.php or a RESTful endpoint, as
configured via $wgPopupsGateway and $wgPopupsRestGatewayEndpoint. A
possible attacker would need to write it's own endpoint (which must either
run on the same server or somehow ignore the CSRF token), and set the
value of mw.config.values.wgPopupsRestGatewayEndpoint on the client to
this endpoint – which requires just *another* attack vector to be able to
do this.
It's "the right thing"(tm) to escape all this anyway.
I found two possibly relevant security reviews of this extension, T88171
and T129177, resolved in 2015 and 2016.
Bug: T88171
Bug: T129177
Bug: T214754
Bug: T214971
Change-Id: I1d118c9ccaea434a253a772d18139b9b077118ab
Instead of maintaining a list of named constants (which must be updated
every time we want to add a new test with a new message), the mock now
behaves like MediaWiki's build-in qqx dummy language code and returns
the message key in brackets. The additional benefit of using the
HTML-like <…> characters is that this will automatically test if the
messages are properly HTML escaped.
Bug: T214970
Change-Id: Id7911036a7b582aff21acf911a826b5421a55938
This will affect all links, including [[Other page#Fragment]] for
example. But it will not have much of an effect there. The mw.Title
class is able to understand strings like "Other page#Fragment". All
old code calls title.getPrefixedDb() on the result. This will *not*
include the fragment. Only the new code will use title.getFragment().
I made sure this does not affect regular page previews, even when the
link is something like [[Other page#Fragment]].
Bug: T213415
Change-Id: I15611a44aa0477cc5e48ee4b12aae3cd981d977c
Mocha discourages the usage of arrow functiones in the test specs since
Mocha context can't be accessed from inside.
I stumbled across this when using this.skip(); in the reference preview
selenium tests. Since it took me some time to figure out why it was not
working, I guess it's better to avoid lambdas generally there.
See https://mochajs.org/#arrow-functions
Change-Id: I95cb183ac88e9a624c449a8f9addbe84bf76c335
I tried hard to keep the CSS as small and robust as possible. The
icon will be align with the text by adding a negativ margin. With
that we also decided against using RTL and LTR specific icons that
are positioned at the edge of the canvas for now.
Bug: T213907
Change-Id: I98888114e1c50e249cf31e71749323bd4f69da3f
I guess both is fine: either having the default in the gateway (as it
was before), or in the renderer (as this patch proposes). I, personally,
feel better with having it closer to where it is needed. This way it's
not possible to accidentially deliver a model object with an empty title.
The renderer will catch this.
At the moment we don't know exactly how we will fetch other titles (e.g.
"Book").
This change is split from I15611a4 where it was a little misplaced.
It also includes a test for the default fallback title.
Bug: T213907
Change-Id: I8ec3ddc21a417da7f95feff7b080cbd60d5472e7
Including tests. I also changed the title to include quotes as well,
even if not critical in that case.
Bug: T214754
Change-Id: I2f92a5714f7adc229a003f9167bcc9afdbc55583
Special characters that have a meaning in one of the many different input
formats jQuery accepts must be escaped.
The real-world use-case are references like <ref name=":1"> with a colon.
But it's many more characters that need escaping. See
http://api.jquery.com/category/selectors/
Note this patch misses a test. I already uploaded I9ec57e0 to fix the
currently incomplete tests. But I can't make it work. How do I create an
element in the test environment so that jQuery finds it?
I suggest to merge this and continue working on the tests later, because
this is currently one of the most annoying issues that makes all testing
unreliable.
Bug: T214710
Change-Id: Ifb5fe896936078f799298ac803d019d9caa048c8
The need for this is more a sign for a broken specification than an
actual issue with this code. But better be sure than sorry. More
details at
https://mathiasbynens.github.io/rel-noopener/
Bug: T214776
Change-Id: Idbcfae6d146fbbe3bff730239329beeb3455e18c
This is documented at http://usejsdoc.org/tags-type.html, but not in many
other places, especially not in the JSDuck documentation.
The {!…} syntax means "can not be null". This is the default anyway.
The {?…} syntax means nullable. In a few situation is was used when a
parameter can be undefined. I decided to remove it everywhere and replace
it with {…|null} when appropriate, because this is much more explicit. Less
syntax to remember.
Note I'm intentionally not using the […] syntax when a parameter is followed
by non-optional parameters. Actually skipping a parameter in such a situation
would mess the parameter order up. Having optional parameters not at the end
is sometimes used as a feature in JavaScript code, but not in this codebase,
as far as I can see.
Change-Id: Ie370cfe08c32d1af5b0341951bed044fc3511c57
I finally found the issue. It was an incomplete mock for the
mw.html.escape() function that would return the string unescaped.
Bug: T213415
Bug: T213908
Change-Id: I198393b3c72771e4018f79913ddb9f4cb2c0d4de
Excluding tests for the renderer which keeps failing. This will be
readded in a later patch.
Bug: T213415
Bug: T213908
Change-Id: If79fa3d0a7a20f121b1ceda6e0e33ad691b1ad30
This does make generic `array` type hints more specific when possible.
I'm also applying my personal best practice to not have any @return
documentation on test @dataProviders. These don't provide any useful
information, and can't. The best type we could use is `@return array[]`,
but that would be the same for every single data provider. Copy pasting
these comments around is of no real value.
Also it was already inconsistent as some did not had this comment.
Change-Id: Id401c7e32493b6a9faaf6d47cddc01e2227102af
The test setup was slightly refactored to be more general about
the type of the popup.
The additional reference links on the test page were added mostly
to be prepared for further tests of more complex cases.
On the CI the tests should be executed with having reference
popups enabled. The code tries to skip test when the feature is
disabled.
See I17687c62cc8d738a4eb41738c9ce6662a5ec68d8
and I1eb7409aa3bd111c2e461dfe245d95f7e78d416c
Bug: T213415
Change-Id: I74110c6227596ff10c75f5f0b0da3d952f11a239
It is more convinient to have the feature enabled by default and set
it to false on the production servers. This makes it easier to use and
test by CI.
Depends-On: I36b4fb7615de5ff6670b70345abf7436538a6828
Change-Id: Iad3bc723e1dd5f79e770c9a2f66190ebcd7c8b9d
It's not exclusively about page summaries any more.
We had a few suggestions in mind:
* get, fetch, request, or issueRequest. But I feel these are all to
generic and don't describe well what the method does. As a reminder:
It expects a Title object and returns a promise, which returns a
PreviewModel object, which contains an HTML "extract".
* fetchPreview? I feel this can still mean to many things.
* fetchPreviewModel? But we don't really need to repeat that it will
return a model object.
So I went for fetchPreviewForTitle. What do you think?
Bug: T213415
Change-Id: Icb32c63cec82f72453dc1507c9f8b8d461fd4f4c
This adds support for preview popups on reference/footnotes from
the Cite extension. For that a new preview type was introduced and
integrated into the existing structures.
The essential starting points were this code comes into action are
added behind the feature flag introduced in the previous patches.
Bug: T213415
Change-Id: Ie0ccb03117bd654373d0f458b62cc52018361c67
Most of the code is exported because is is tested separately. But
these are all tested via createPreviewWithType(). I think it's just a
minor mistake to have these exported.
Change-Id: Ic4f4dc40fd95a60aba45cb5aa3fcbb6e3bc8c386
This is a prerequisit for the later patch Ie0ccb03.
Any JavaScript code can check this feature flag via
mw.config.get( 'wgPopupsReferencePreviews' )
Bug: T213415
Change-Id: I17687c62cc8d738a4eb41738c9ce6662a5ec68d8
Output a timestamp whenever a Webpack build completes. This is useful
for verifying development builds generated by the `npm start` watcher
are up-to-date.
If you merge this patch, merge the corresponding patch to the
MobileFrontend Webpack config.
Change-Id: Ifeb96506475bacc2b97a24d218a28daa55dea137
This is split from the current draft patch Ie0ccb03. This is part of a
series of very small patches that prepare the code for new types of popups.
We decided to not add code for other types of popups to the existing
createGateway() function, but introduce new files and functions instead.
Renaming, for example, the existing `gateway` variable name will make it
much more obvious which of the future gateways does what.
Bug: T213415
Change-Id: Ifcbc3ba53d0ab9ef67adf1f314defc76b4f89e89
This is split from the current draft patch Ie0ccb03. This is part of a
series of very small patches that prepare the code for new types of popups.
Bug: T213415
Change-Id: I4f4392057f6d3eff78409c8b6f49898c8be45d3e
This became an issue after the patch Ifa56d41 (part of T206323) removed
some hidden <span> and replaced them with title="…" tags. The CSS selector
in src/index.js get's active on all <a> elements that have a title="…"
attribute. This is now the case for all references that are used one time
and have their ↑ (or ^ on the English Wikipedia) linked.
I realized this is really only an issue for these ↑ links.
The more general issue described in T198652 still holds true, but becomes
less urgent with this bugfix.
Bug: T198652
Bug: T206323
Bug: T212419
Change-Id: I9287e8692d031f9d2ba50f967520bf327ed5c42f
It is probably not that critical to understand if "09/11/2016" refers
to September 11th or November 9th. I still think it's worth looking for
a documentation format that is easy to understand by an international
developers community. My personal best practice is to use the ISO format.
Change-Id: Ib209a8a1651970d74f82c188ae4b84d1a4eb534e
This is split from the current draft patch Ie0ccb03. This is part of a
series of very small patches that prepare the code for new types of popups.
Bug: T213415
Change-Id: I00d46a716c0e6ada82ffc0034a7dd5582363c657
This makes the following code work much better in IDEs, e.g. it's now
possible to click method names and actually go there.
Change-Id: Iddbe838f1402c5017ffeb3bc03b4a017b05d8b02
