Page MenuHomePhabricator

Added a setup check for empty REMOTE_ADDR
ClosedPublic

Authored by jcox on Aug 25 2016, 4:32 PM.
Tags
None
Referenced Files
F12850678: D16450.id39566.diff
Fri, Mar 29, 5:44 AM
F12847428: D16450.id39567.diff
Fri, Mar 29, 2:12 AM
Unknown Object (File)
Jan 31 2024, 4:44 PM
Unknown Object (File)
Jan 19 2024, 8:28 PM
Unknown Object (File)
Jan 19 2024, 8:28 PM
Unknown Object (File)
Jan 10 2024, 6:12 AM
Unknown Object (File)
Dec 27 2023, 8:47 AM
Unknown Object (File)
Dec 27 2023, 8:47 AM

Details

Summary

Fixes T8850. Previously, if a user's preamble script mangled $_SERVER['REMOTE_ADDR'] or somehow set it to null, the user would get errors when performing certain actions. Now those errors shouldn't occur, and instead the user will be warned that there is a setup issue related to their preamble script.

Test Plan

Create a preamble script that contains $_SERVER['REMOTE_ADDR'] = null; then navigate to /config/issue/. There should be a warning there about REMOTE_ADDR not being available.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jcox retitled this revision from to Added a setup check for empty REMOTE_ADDR.
jcox updated this object.
jcox edited the test plan for this revision. (Show Details)
epriestley added a reviewer: epriestley.
epriestley added inline comments.
src/applications/config/check/PhabricatorPHPConfigSetupCheck.php
212–221

This sort of fragment ("%s and double-check") may be difficult to translate into other languages, since they may want to change word order or need to adjust articles depending on noun gender.

A rephrasing that uses fewer fragments, maybe like this one, may be easier to translate in a natural-sounding way:

Consult the documentation (%s) and double-check....

Where the replacement is just a link titled "Configuring a Preamble Script".

(This is maybe-possibly slightly better for assistive technology / screenreader stuff too, since it makes "read all links on this page" more meaningful, although probably not too substantively.)

219

Just omit 'rel' -- we automatically manage it (currently, by adding 'noreferrer' to non-local links) and specifying it will override whatever behavior we're implementing.

(It's possible that we should change the behavior, e.g., to include noopener, although the use-window.opener-to-do-phishing-stuff attack doesn't currently seem hugely concerning to me. If you want to make an argument for changing the defaults, feel free to file a task.)

This revision is now accepted and ready to land.Aug 25 2016, 4:51 PM
jcox edited edge metadata.
  • Updated paragraph to be more translatable
jcox marked 2 inline comments as done.Aug 25 2016, 4:57 PM
This revision was automatically updated to reflect the committed changes.