Page MenuHomePhorge

Remarkup: harden how we recognize internal/external URIs (mailto, mobile phones, ...)
AcceptedPublic

Authored by valerio.bozzolan on Dec 5 2024, 15:52.
Referenced Files
F3938445: D25847.1746428304.diff
Sun, May 4, 06:58
F3801345: D25847.1745948434.diff
Mon, Apr 28, 17:40
F3749192: D25847.1745791933.diff
Sat, Apr 26, 22:12
F3741475: D25847.1745765008.diff
Sat, Apr 26, 14:43
F3733129: D25847.1745744043.diff
Sat, Apr 26, 08:54
F3720984: D25847.1745717470.diff
Sat, Apr 26, 01:31
F3720982: D25847.1745717465.diff
Sat, Apr 26, 01:31
F3720981: D25847.1745717462.diff
Sat, Apr 26, 01:31

Details

Summary

Some unit tests were missing the new CSS class 'remarkup-link-ext'. Added.

The CSS class was introduced here:

https://we.phorge.it/rP349f006904fabf1d4df31ff4840502af3ab379a7

https://we.phorge.it/D25118

Also, Remarkup is now hardened a bit, to do not consider anymore these as internal links:

  • mailto:info@example.com
  • mobile phone numbers
  • alien protocols

Also, Remarkup engine option 'uri.base' is now taken in consideration, to know if an URI is internal or not.

Closes T15967
Closes T15973
Closes T15974

Test Plan

Run these unit tests, and they finally work again:

arc unit ./src/infrastructure/parser/__tests__/PhutilPygmentizeParserTestCase.php
arc unit ./src/infrastructure/markup/remarkup/__tests__/PhutilRemarkupEngineTestCase.php

Additionally, run this, that has new tests added:

arc unit ./src/infrastructure/parser/__tests__/PhutilURIHelperTestCase.php

Also run this, check that the class tree is already correct:

arc liberate

Have fun with the green lights.

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25847_3
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1927
Build 1927: arc lint + arc unit

Event Timeline

aklapper subscribed.

I applied this patch locally on top of git master and output does not complain anymore about 'link-brackets.txt' (thus it's correct) but fails in link-edge-cases.txt now (thus it's likely not complete):

   FAIL  PhutilRemarkupEngineTestCase::testEngine
Assertion failed, expected values to be equal (at PhutilRemarkupEngineTestCase.php:66): Failed to markup HTML in file 'link-edge-cases.txt'.
Expected vs Actual Output Diff
--- Old Value
+++ New Value
@@ -1,11 +1,11 @@
-'<p><a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a></p>
+'<p><a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a></p>
 
-<p>(<a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a>)</p>
+<p>(<a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a>)</p>
 
-<p><a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a></p>
+<p><a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a></p>
 
-<p><a href="http://www.example.com/wiki/example_(disambiguation)" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/wiki/example_(disambiguation)</a></p>
+<p><a href="http://www.example.com/wiki/example_(disambiguation)" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/wiki/example_(disambiguation)</a></p>
 
-<p>(example <a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a>)</p>
+<p>(example <a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a>)</p>
 
-<p>Quick! <a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a>!</p>'
+<p>Quick! <a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a>!</p>'
This revision is now accepted and ready to land.Dec 5 2024, 17:38

but fails in link-edge-cases.txt now (thus it's likely not complete):

You are right. Marking as revision not ready.

But this is becoming a rabbit hole. Seems the Env/Configs are not configured correctly in unit tests, and we really need them:

arc unit src/infrastructure/markup/remarkup/__tests__/PhutilRemarkupEngineTestCase.php 
   FAIL  PhutilRemarkupEngineTestCase::testEngine
EXCEPTION (Exception): Trying to read configuration "phabricator.production-uri" before configuration has been initialized.
#0 /home/boz/repos/phorge/src/infrastructure/env/PhabricatorEnv.php(423): PhabricatorEnv::getEnvConfig('...')
#1 /home/boz/repos/phorge/src/infrastructure/env/PhabricatorEnv.php(447): PhabricatorEnv::getProductionURI('...')
#2 /home/boz/repos/phorge/src/infrastructure/env/PhabricatorEnv.php(441): PhabricatorEnv::getSelfURIMap()
#3 /home/boz/repos/phorge/src/infrastructure/parser/PhutilURIHelper.php(43): PhabricatorEnv::isSelfURI('...')
#4 /home/boz/repos/phorge/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php(80): PhutilURIHelper->isSelf()
#5 /home/boz/repos/phorge/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php(186): PhutilRemarkupDocumentLinkRule->renderHyperlink('...', '...')
#6 /home/boz/repos/phorge/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php(133): PhutilRemarkupDocumentLinkRule->markupDocumentLink(Array)
#7 [internal function]: PhutilRemarkupDocumentLinkRule->markupAlternateLink(Array)
#8 /home/boz/repos/phorge/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php(17): preg_replace_callback('...', Array, '...')
#9 /home/boz/repos/phorge/src/infrastructure/markup/blockrule/PhutilRemarkupBlockRule.php(92): PhutilRemarkupDocumentLinkRule->apply('...')
#10 /home/boz/repos/phorge/src/infrastructure/markup/blockrule/PhutilRemarkupDefaultBlockRule.php(17): PhutilRemarkupBlockRule->applyRules('...')
#11 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/PhutilRemarkupEngine.php(283): PhutilRemarkupDefaultBlockRule->markupText('...', NULL)
#12 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/PhutilRemarkupEngine.php(140): PhutilRemarkupEngine->markupBlock(Array)
#13 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/PhutilRemarkupEngine.php(106): PhutilRemarkupEngine->preprocessText('...')
#14 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/__tests__/PhutilRemarkupEngineTestCase.php(56): PhutilRemarkupEngine->markupText('...')
#15 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/__tests__/PhutilRemarkupEngineTestCase.php(13): PhutilRemarkupEngineTestCase->markupText('...')
#16 /home/boz/repos/arcanist/src/unit/engine/phutil/PhutilTestCase.php(639): PhutilRemarkupEngineTestCase->testEngine()
#17 /home/boz/repos/arcanist/src/unit/engine/PhutilUnitTestEngine.php(69): PhutilTestCase->run()
#18 /home/boz/repos/arcanist/src/unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php(148): PhutilUnitTestEngine->run()
#19 /home/boz/repos/arcanist/src/workflow/ArcanistUnitWorkflow.php(170): ArcanistConfigurationDrivenUnitTestEngine->run()
#20 /home/boz/repos/arcanist/scripts/arcanist.php(427): ArcanistUnitWorkflow->run()
#21 {main}

I mark as changes planned, but I really don't know what to do.

Let's talk in new task T15973: Fix unit test PhutilRemarkupEngineTestCase

also tried to fix PhutilRemarkupEngineTestCase

This revision is now accepted and ready to land.Dec 9 2024, 09:51
valerio.bozzolan retitled this revision from Fix unit test PhutilPygmentizeParserTestCase to Fix regression in Remarkup unit tests.
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited the test plan for this revision. (Show Details)
valerio.bozzolan retitled this revision from Fix regression in Remarkup unit tests to Fix regression in Remarkup unit tests and harden.Dec 9 2024, 11:21
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited the test plan for this revision. (Show Details)
valerio.bozzolan retitled this revision from Fix regression in Remarkup unit tests and harden to Fix regression in Remarkup unit tests.
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited the test plan for this revision. (Show Details)

\o/

This revision is now accepted and ready to land.Dec 9 2024, 11:21
valerio.bozzolan retitled this revision from Fix regression in Remarkup unit tests to Fix regression in Remarkup unit tests and harden.Dec 9 2024, 11:42
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited the test plan for this revision. (Show Details)
valerio.bozzolan added inline comments.
src/infrastructure/env/PhabricatorEnv.php
14

↑ Legacy documentation error.

valerio.bozzolan added inline comments.
src/infrastructure/markup/remarkup/__tests__/remarkup/link-square.txt
19

This is considered internal. Seems strange. Better to double-review.

add more edge cases in unit tests

This revision is now accepted and ready to land.Sat, May 3, 13:02
src/infrastructure/parser/__tests__/PhutilURIHelperTestCase.php
25–32

Now these ↑ are always executed, I've moved out the if condition.

valerio.bozzolan retitled this revision from Fix regression in Remarkup unit tests and harden to Fix minor regression in Remarkup unit tests and harden.Sat, May 3, 13:11
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan retitled this revision from Fix minor regression in Remarkup unit tests and harden to Remarkup: harden how we recognize internal/external URIs (mailto, mobile phones, ...).Sat, May 3, 13:13

transaction.search: handle unmanaged exception when objectType is not supported

Closes T16054

Test Plan:

Visit the page /api/transaction.search, fill the objectType with "XACT" and search:

You do not see anymore this confusing exception:

get_class() expects parameter 1 to be object, null given

Instead, you see this exception message:

In call to "transaction.search", specified "objectType" ("XACT") exists but is not supported. Try with a supported value like "TASK".
src/infrastructure/parser/PhutilURIHelper.php
85–86

Is there a particular reason why this is not $this->extraTrustedDomains[] = $uri->getDomain()? Why set 1 as a value, it seems irrelevant? (Also applies to PHPDoc above)

valerio.bozzolan added inline comments.
src/infrastructure/parser/PhutilURIHelper.php
85–86

Love this question. I usually do search-by-key because it's adorably blazing fast, O(1), that means TA-DAH, at least compared to the search-by-value that is slower. So the line 108 benefit from this, with less code also.

https://stackoverflow.com/a/13483548/3451846

(I was also almost sure that Phorge was doing these things, cannot find interesting links right now lool)

I will add a comment so is more self-evident thanks

valerio.bozzolan marked an inline comment as done.

add some inline documentation about $array[$v] = 1

also git rebase origin/master just because we can

Thanks andre