Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
during file uploads, string length of a file's destination path($file->filepath) is not checked. only filename(basename) is validated. since filepath is stored as varchar(255), it needs to be validated or have more db storage space. i dont know which way is the best but filepath validation will eliminate the need for basename validation.
Comment | File | Size | Author |
---|---|---|---|
#52 | Screen Shot 2023-01-24 at 8.31.24 pm.png | 73.19 KB | pameeela |
#30 | filename_and_uri_length.patch | 4.23 KB | mr.baileys |
#25 | filepath_2k_7.patch | 4.83 KB | bellHead |
#18 | filepath_2k_6.patch | 3.72 KB | bellHead |
#15 | filepath_2k_5.patch | 3.21 KB | bellHead |
Comments
Comment #1
Wim LeersComment #2
ufku CreditAttribution: ufku commentedwhy should we postpone checking a string's length?
Comment #3
mr.baileysIt's not postponing. Bugs are fixed against HEAD (7.x-dev) and then backported to previous versions.
Comment #4
drewish CreditAttribution: drewish commentedhumm... good point we might need to add another default validator that checks that the file path fits.
Comment #5
codecowboy CreditAttribution: codecowboy commentedThis should be revisited. We still have the 255 character limit on file.uri. The URI in d7 should in general be shorter than earlier versions, but this could still be an issue. Is there any validation in place?
Comment #6
codecowboy CreditAttribution: codecowboy commentedbased on http://www.boutell.com/newfaq/misc/urllength.html
and the fact that the first stable release of mysql 5 was 5.0.3 which supports varchars up to 2^16, postgres not caring about the length of the varchars, sqllite having a billion character limit to varchars by default, mssql supports massive nvarchars. Apache supports up to 8192 bytes in the http header fields and UTF-8 potentially using up to 3 bytes per character.. 2730 is the closest neighor in characters..
length => 2048 with a validate function?
Comment #7
codecowboy CreditAttribution: codecowboy commentedhere a first go with a validation function and the schema change.
Comment #9
codecowboy CreditAttribution: codecowboy commenteddid some more poking around.. apparently http headers are only supposed to use ascii characters which means only 1 byte per character so the division by 3 for UTF-8 is unnecessary... I'll reroll with the system.install fixed tonight.
Comment #10
sun.core CreditAttribution: sun.core commentedSeems like another patch got mixed in here.
I don't think this works.
Powered by Dreditor.
Comment #11
bellHead CreditAttribution: bellHead commentedRerolled the pertinent bits.
In a public file access model the practical limit is not what the servers can handle (which is configurable anyway), but what clients can handle. The entire IE family up to IE8 have a path length limit of 2048 characters.
Comments updated to reflect this rather than header lengths and encoding sizes as the practical basis for the limitation.
Comment #12
bellHead CreditAttribution: bellHead commentedBot didn't like it. Will update later today.
Comment #13
bellHead CreditAttribution: bellHead commentedsystem_update_7040() had been used elsewhere in the meantime. Moved updates to the file table to update_fix_d7_requirements() in keeping with the comments in other system_update_* functions in system.install.
Key length limit for mysql 5.1 is 786 bytes, so the unique constraint is not longer enforcable on uri. Creation of this constraint has been removed from system.install and it is dropped in update_fix_d7_requirements().
bellHead vs bot Round 2 :)
Comment #14
catchI'm sure we have some queries on uri, so if the unique constraint must be removed, then a non-unique index must be added to replace it.
Comment #15
bellHead CreditAttribution: bellHead commentedIndex added for uri in all the relevant locations.
Comment #16
eojthebraveThis seems like a good idea to me. Just a couple of minor code style issues.
Just being picky here but can we change "uri" to "URI" in the documentation and user facing messages.
Documentation should wrap at 80 characters.
Powered by Dreditor.
Comment #17
mr.baileysShouldn't we be checking against the limit (2048)? If not, we should document why these last 8 bytes are reserved.
Comment #18
bellHead CreditAttribution: bellHead commentedMySQL 5.0.1 and 5.0.2 where alphas, 5.0.3 up to 5.0.14 where betas, it only went production at 5.0.15. I don't think it's too much of a leap to read an implied 'production version of' in the requirements.
I rerolled the patch from codecowboy's efforts, so I left the length check mismatches in place in case there was a reason behind them. There is another weird one in the same vein, filenames are checked at 240 not 255 without any explanation as well. My only guess is that the 240 is based on ideas about maximum filename lengths on Windows, even though I can't find any official support for the number. All the operating systems IIS5 runs on do 255 char filenames.
There doesn't seem to any reason to reserve the space in these fields - data is written into them without any kind of wrapper. So I've bumped both those length checks out to match the field lengths.
EDIT: I've discovered why space is reserved in URI, but no answers on filenames. Updates once I've figured out some more details.
Comment #20
catchOpened #698902: Make MySQL 5.0.15 requirement official - seems like we might as well bump the version number a bit.
Patch looks fine apart from two things
Somewhere there was an issue to removing type hinting for stdClass in case people want to do crazy things in contrib making entities real classes. However I can't see it now and not sure if it even went in.
vim typo?
Powered by Dreditor.
Comment #21
bellHead CreditAttribution: bellHead commentedThe patch for this issue has gotten deeply confused, and probably serves as an instructive example of what awaits those who try to pick up abandoned issues without doing all the homework.
We now know that ...
The issue was originally raised about truncation or insert failures of the paths in {files} under d6.
Under d7 the issue is somewhat different. The filepath field has been removed and a field called uri has been added. The value stored in uri may be shorter than what was stored in filepath since it is relative to the base of the Drupal installation rather than being an absolute path. It may also be longer since it now includes the filename and a pseudo-protocol 'public' or 'private'.
Truncating values (on a permissive mysql installation) under d7 is as much of a risk as under d6, and will lead to files not being retrievable in either case.
The externally imposed constraints are:
The answer of lengthening the field is not practical under d6 since it is required to support databases which have a size limit of 255 on char fields.
The IE family of browsers up to at least version 8 limit the length of the path portion (the bit between the host and the filename) of a URL to 2048 characters.
The practical limit on the filename is 255 characters imposed by Windows.
The 260 character fully qualified filename (path + name) limit on Windows does appear to apply to at least the earlier versions of IIS supported. It doesn't give an error, it just returns 404 for files that break the limit.
Since this last one is the most restrictive limit, and since by disrupting the local access to file it affects private and public storage equally this is the restriction that matters.
What this means for an answer:
- The lengths of paths for file storage for upload fields need to be checked when the fields are created, these should be limited at 220 so that there are least 40 characters for file naming.
- Filename length checks need to be done against the path and filename limit of 260
Thoughts?
I'll start rolling a patch to this effect later in the week.
Comment #22
catchIf it's II6 that's the lowest limitation, then I don't think we should worry about that for D7 - it's no longer supported by Microsoft, and there are two more recent releases, or that's what it looks like according to http://support.microsoft.com/lifecycle/?p1=3198
Comment #23
bellHead CreditAttribution: bellHead commentedI got the restrictive results on IIS5.1 (Windows XP), tests on IIS7 (Vista) seem ok, but I don't have a Server 2003 to test IIS6 on.
Support for XP is already over, Server 2003 will be supported for around three months after the planned release date of Drupal 7 according to
http://support.microsoft.com/lifecycle/search/default.aspx?sort=PN&alpha...
although this may differ by territory.
There are big hopes for faster uptake of D7 than of D6 - if IIS6 is as restrictive as IIS5 is this time window narrow enough that it can just be ignored?
Some test results from IIS6 would of course be very useful. If it's not as restrictive as IIS5.1 then there is no issue.
Test procedure is basically:
- Create a site which allows directory browsing (It's easier than all the copy and paste)
- Create a directory within that site with any name
- Create a text file in the directory with a 100-120 character name
- You should now be able to browse to the file and open it
- Rename the directory to a name 220 characters long
- You should be able to browse to the directory unless the path to the site is already very long
- If you can't browse to the directory or you get a 404 on opening the file IIS is unable to handle the filename
- You may also get at 414 error (Request URL too long) - I don't think this should happen on IIS6
- Hopefully you can still open the file :)
Comment #24
webchickUnless someone can think of a really compelling reason not to do so, I think Drupal 7 not supporting IIS 6 is fine. IIS users make up only a very tiny fraction of our userbase, so they get the same rules applied on them as other minimal technologies, IMO. We don't go out of our way to support Opera 2 either.
It's also fairly consistent, since we also dropped support for older versions of other technologies, including PHP 4, and MySQL 4 and PostgreSQL 7 this release.
Comment #25
bellHead CreditAttribution: bellHead commentedTo work under IIS7 and up, and on MySQL 5.0.15 and up. 2048 characters for file path, 255 for the filename and 10 for the pseudo-protocol add up to the 2313 field length for URI.
Comment #26
sunerrr, varchar > 255? If this is possible at all, then this needs an inline comment, potentially pointing to online resources that prove this.
Powered by Dreditor.
Comment #27
mr.baileysvarchars > 255 is no problem, see earlier comments in this issue (the MySQL version requirement for D7 was changed as a result of this issue). I don't think this needs an inline comment, but it would probably be a good idea to include a test that exceeds the 255 char limit.
Comment #28
drewish CreditAttribution: drewish commentedI really want to protest the removal of the unique key in #13. Its entire purpose is to prevent the same file from ending up with two file ids. If the unique constraint tops out at 768 bytes then I'd suggest using that as a maximum field length.
Comment #29
codecowboy CreditAttribution: codecowboy commented768 characters would be good enough for most cases. It's 3x what is currently available, and we're looking for the maximum theoretically functional length and the unique constraint on file.uri is an important part of the specification for files.
does http://abcdefghi/abcdefghi/abcdefghi/abcdefghi/abcdefghi/abcdefghi/abcde... look long enough for most use cases.. Thats a little over 700 characters...
I think this is one of those cases where protecting the data has to trump potential. At least we're aware of the feasible limits should mysql increase it's unique constraint length.
Comment #30
mr.baileysRe-rolled with the 768 limit imposed by MySQL maximum unique key size, which seems to be the lowest common denominator.
Comment #32
catchtagging.
Comment #33
marcingy CreditAttribution: marcingy commentedAfter some digging I think I have come to a conclusion as to why this path fails to result in a successful install. The maximum size for a varchar that will serve as an index which uri does is 767 bytes under InnoDB. However as we use UTF-8 each character accounts for 3 bytes, so any varchar set to be greater than 255 will result in a maximum key size violation. See http://bugs.mysql.com/bug.php?id=4541 for more information on the issue.
So unless we are going to store some sort of hashed representation of the uri, remove the unquie index or convert uri to text the maximum length we have to play with is varchar(255).
Comment #34
catchSo either this is won't fix, or we need to validate the upload length if we're not already. Either way I'm downgrading this issue from critical, since it's just a normal bug.
Comment #35
marcingy CreditAttribution: marcingy commentedSetting to won't fix
Comment #36
aliceten CreditAttribution: aliceten commentedWell, you can use Long Path Tool, it works perfectly.
Comment #37
stefan.r CreditAttribution: stefan.r commentedPeople still run into this:
https://drupal.stackexchange.com/questions/36760/overcoming-255-characte...
#2374529: Data too long for column uri
#193954: {file}_uri and {file}_filename length limitations
#70201: Aggregator: Feed URL Length Is A Limitation
#2386539: Varchar(255) too small for some long url
Maybe this was because of http://www.w3.org/2001/tag/doc/get7#myths?
The problem was that the unique constraint didn't allow for more than 255 characters. In utf8mb4 this is even 191 characters, so in #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols) we proposed to save a hash of the file URI and use a unique constraint on that instead. If we go ahead with that, it should allow us to save longer URIs...
Comment #38
stefan.r CreditAttribution: stefan.r commentedComment #50
quietone CreditAttribution: quietone at PreviousNext commentedThis was a bugsmash group triage issue. It was discussed with lendude and myself.
There has been no discussion here for 8 years. Perhaps this has been fixed in the meantime?
Lendude thinks this may have been fixed by #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols) and that #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) seems relevant.
Is this issue still relevant?
Since we need more information to move forward with this issue, I am setting the status to Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!
Comment #52
pameeela CreditAttribution: pameeela at Technocrat commentedSeems it has been fixed, if I'm understanding correctly: