robots.txt doesn't need to be passed through check_plain(). Normally, when robots.txt is served from a file (that is what happens in all the Drupal installations which don't use robotstxt.module), it is given as it is, without to be preprocessed before.

The call to check_plain() is also useless because:

  • The content of robots.txt is valid UTF-8, therefore the call to drupal_validate_utf8() (which is called from check_plain()) is useless; in the case the content of the file would not be valid UTF-8 (I doubt it is normally possible), it's enough to open it in a editor, and save it using the right encoding.
  • The file content is plain text, therefore the call to htmlspecialchars() (which is the other function called from check_plain) is not required, also because the file is not embedded into HTML content; if then a folder, or a file listed in robots.txt would contain a &, this would be transformed in &, with the result that a line like
    Disallow: /hide&seek.txt would be changed in Disallow: /hide&seek.txt; the result is that the search engines would avoid to index a file that doesn't exist, and would index a file that was supposed to not be indexed.
CommentFileSizeAuthor
#14 Capture_093431.png64.2 KBavpaderno
#14 Capture_094545.png74.56 KBavpaderno
#14 Capture_096631.png86.51 KBavpaderno

Comments

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: robots.txt doesn't need to be passed to check_plain() » robots.txt content doesn't need to be passed to check_plain()
hass’s picture

hass commented
Status: Postponed (maintainer needs more info) » Active

I will check this out. As it could break URLs I mark this a bug.

Aside If we do not filter anymore - someone could add arbitrary code into the robots.txt and this may get executed by the browser...

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Category: task » bug

If that would be the case, then it would happen also in a plain Drupal installation, as in such case the content of robots.txt is not passed through check_plain().

The settings for RobotsTxt are not changed by all the users who have access to a web site, but only from who has the right permission; if there is somebody with the right permission who, through RobotsTxt, adds wrong content to robots.txt, then the problem is to give the permission to change the module settings to the wrong person.

hass’s picture

hass commented

The major difference between the core file and our output is - the robotstxt stuff is user configurable. User configurable stuff should be checked - nevertheless you are right that is is more or less a permission thing and the trust to persons. Otherwise - changing the core file normally requires root permission and SSH/FTP access to the installation... maybe we can build a function like check_plain that kills all code except allowed code. Something like an XSS/content filter... we will see.

hass’s picture

hass commented

PS: We also shouldn't forget the people with WYSIWYG editors wrongly configured :-(((. Popped up often in all my queues... <-- reminder that we should also add 'WYSIWYG' = FALSE toe the robotstxt area while we are on it...

hass’s picture

hass commented

Funny... since 11.3.2006 (first commit of the module to CVS) there is a check_plain on the output...

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

When the module is called in action, it returns what the user with right permission wants to be the content of robots.txt, and it sets the content type to be text/plain (drupal_set_header("Content-type: text/plain");); the content returned could also contain HTML, but it would be treat like plain text.
robots.txt is normally used by the search engines to know which directories (or files) they are allowed to scan, and they will treat the content like plain text; therefore nothing wrong would happen to them.

I could even put some JavaScript code into the content returned by the module, but it will never be executed from a web browser, also because the file the web browser thinks to see has extension .txt (that added to the fact that the server returns plain/text like content type).

hass’s picture

hass commented
Status: Active » Postponed (maintainer needs more info)

I tried now over 2 hours to find any valid resource/documentation/example that tells me if & or &amp; should be used in robots.txt. I was unsuccessful. If someone can provide documentation for this ultra edge case it would be good, but I'm not going to change this for now until we have found a documentation that is proven to be correct nevertheless it might be wrong today. Also robotstxt.org doesn't have such an example... I believe I would only trust resources from google/yahoo/msn documentation pages or similar.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Maybe there isn't any documentation on that because robots.txt is just a plain text file.

hass’s picture

hass commented

Maybe the current approach is not wrong? We cannot say for sure today...

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

What I mean is that, as the module set the content type like plain text, the browser (in the remote case a user would point his browser to http://example.com/robots.txt) will treat the content like plain text, and it will not try to render it like HTML; therefore a function like htmlspecialchars() would not have any use.

Suppose then that I use RobotsTxt to add a line like Disallow: hide&amp;seek; that is perfectly allowed, but the call to check_plain() will convert it to Disallow: hide&amp;amp;seek.

hass’s picture

hass commented

You don't need to repeat yourself 4-5 times. Please find a reference that makes sure we do something wrong - otherwise it may be correct, but it may also be wrong. We cannot say for sure nevertheless it's a plain text file.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

I could not really reply to the question, as I would repeat myself.
The menu callback which is associated with the URL robots.txt has a line that calls drupal_set_header() passing 'Content-type: text/plain' like parameter.
If you want to be sure it's really plain text, try add a HTML fragment in it, and see what the browser shows you.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
StatusFileSize
new Capture_096631.png86.51 KB
new Capture_094545.png74.56 KB
new Capture_093431.png64.2 KB

I changed the code in the module, and removed the call to check_plain() made by the menu callback.
I then added an HTML tag, and checked what the browser would show me.

The attached files show the result in different web browsers (Safari, Camino, Firefox). The HTML fragment I added is shown by three different browsers like plain text (as it should be). The callout has been added by me, not by the browsers.

hass’s picture

hass commented

Nevertheless the content type is text/plain I'd like FIRST to see a documentation that explains how we need to add URLs with & into robots.txt files. I don't like to have theoretical guessing discussions. You also don't need to tell me what HTML is or not and if it's executed by a browser or not if the extension is .txt.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

It doesn't seem a theoretical guessing.

The report is about check_plain() being used when it's not really needed. As the browsers would not show the content passed to them from the module like HTML, what is the purpose to pass the module output through a function that calls htmlspecialchars()? The search engines will never use the content like HTML too.

Forget for once the &, which is the last problem. The problem is calling a function that is not really needed.

AlexisWilke’s picture

AlexisWilke commented

hass,

Maybe you shouldn't be the one working on this module if you cannot agree with Kiam on this one.

robots.txt is text and only robots are reading it. It won't be executed by Apache (or whatever server you have) unless you change your setup to execute a .txt file (who will in their right mind?!)

The check_plain() is not correct. And the data in the file needs to be pure text (no WYSIWYG, although that you cannot really control).

There is no special character to handle in a text file. The content is just the same as what you type in your URL text box at the top of your browser. That's all.

check_plain() is important when you send HTML only, not text. Do you eventually use check_plain() on images? Like you know if you were to send a PNG with a & in it... that could be dangerous, right? Same difference.

Thank you.
Alexis Wilke

hass’s picture

hass commented

@AlexisWilke: It seems like you also have not read what I've written above or both have problems to understand me. As already said - I'd like to first see a reference and then we change it - for sure.

robertDouglas have added the check_plain and I'm not sure *why* as this is nearly 4 years ago and nobody else found this bug yet. I don't like to change from wrong to wrong nevertheless I *believe* that a check_plain may be wrong. We need to know the right way first without guessing! I'm also not yet sure if URLs with & in robots.txt are really supported... what is the main issue about "know the right way for sure" - change afterwards.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Active » Postponed (maintainer needs more info)

I still have to understand what can be wrong on not using a function that is not thought to be used on plain texts.
check_plain() is thought to be used when an arbitrary text is inserted in HTML output. In this case, the output of the module is set to plain/text, and therefore the function is not of any help.

I have never seen a browser trying to execute the content of a plain text. The only way to make the browser execute such content is to change the content type associated with it.

What hass said about being in two not understanding him reminds me of the joke of the man driving in the wrong side.

AlexisWilke’s picture

AlexisWilke commented

hass,

The root documentation for robots.txt is here:

http://www.robotstxt.org/robotstxt.html

Google has information here:

http://www.google.com/support/webmasters/bin/answer.py?answer=35237

One of the comments about Google is:

The MIME type - if the file is a type other than text, we can't process it.

If you want a definitive answer, I suggest that you use the Google tools. You will need an account with them, but that will answer your question for sure. They have Googlebot parser for the robots.txt file that will do the work for you.

Thank you.
Alexis Wilke

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

(04:50:02 PM) davereid: Does anyone know, on a *plain* text output like robots.txt, do we need to run it through check_plain()?
(04:50:20 PM) merlinofchaos: davereid: It's not the output that matters, it's the input.
(04:50:29 PM) tha_sun_: davereid: user-defined values, always
(04:50:33 PM) merlinofchaos: davereid: If the input came from an untrusted source, then yes.
(04:51:07 PM) davereid: merlinofchaos: tha_sun_: Even if the file is being served with "Content-type: text/plain" and an exit() is called right after the output?
(04:51:29 PM) tha_sun_: davereid: well, heh, that's questionable :P
(04:52:13 PM) merlinofchaos: Hm.
(04:52:26 PM) merlinofchaos: I guess it won't get parsed as HTML then.
(04:52:29 PM) davereid: merlionofchaos: tha_sun_: For example, a configurable robots.txt variable that has the url "Disallow: foo&bar=ferzle" turns it into "Disallow: foo&bar[]=ferzle"
(04:53:40 PM) tha_sun_: davereid: When only putting into robots.txt, I think you can skip check_plain()/check_foo(), but if you display those values somewhere (admin?), you have to escape
(04:54:05 PM) tha_sun_: Robots should have their own sanization... ;)
(04:54:37 PM) davereid: tha_sun_: Yeah I agree. It seems trivial to run check_plain() on the raw output in this case.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Looking through more docs. I'd say the most definitive answer we can get is the actual file at http://google.com/robots.txt:

Allow: /news?btcid=
Disallow: /news?btcid=*&
Allow: /news?btaid=
Disallow: /news?btaid=*&
Disallow: /setnewsprefs?
Disallow: /index.html?
Disallow: /?

That same section run through check_plain() would be:

Allow: /news?btcid=
Disallow: /news?btcid=*&amp;
Allow: /news?btaid=
Disallow: /news?btaid=*&amp;
Disallow: /setnewsprefs?
Disallow: /index.html?
Disallow: /?

In addition, I have found several pages referencing using query strings in robots.txt:
http://sanzon.wordpress.com/2008/04/29/advanced-usage-of-robotstxt-w-que...
http://www.highrankings.com/forum/index.php?showtopic=33643
http://forums.seochat.com/seo-test-and-experimentation-81/blocking-dynam...
http://www.google.com/support/webmasters/bin/answer.py?hl=en&answer=40360#1

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

That is exactly the point. The content would be changed, if passed through check_plain(); then, the search engine would not translate the HTML entities back to the original characters.

I would not expect too much, about the solution of this issue. You should just notice when the report has been open, and when somebody else reported what I was already saying from the beginning; I don't pretend that somebody changes something just because I say so, but it's passed one month from when somebody else commented about this report.

hass’s picture

hass commented
Status: Postponed (maintainer needs more info) » Fixed

@Dave Reid: thank you for the examples and links.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Thanks hass. I'm glad we could finally agree on a decision. This should also improve performance for the module as well. :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.