error checking around record creation?

webrecorder / warcio

Streaming WARC/ARC library for fast web archive IO

https://pypi.python.org/pypi/warcio

Apache License 2.0

385 stars 58 forks source link

error checking around record creation? #40

Open wumpus opened 6 years ago

wumpus commented 6 years ago

Given this whitespace-related header bug that crept into the August 2018 Common Crawl crawl , it would be nice if it was somewhat difficult to create broken WARC files using warcio.

I see a couple of possible issues:

The programmer could pass in http_headers that have trailing CR or LF
The programmer could pass in warc_headers or warc_headers_dict that have trailing CR or LF
These bad things could happen in warcwriter.py in create_warc_record()
These bad things could happen in recordloader.py in the ArcWarcRecord constructor

ikreymer commented 6 years ago

I suppose a check for trailing CR/LF in headers could be added to prevent accidentally invalid headers. Then again, this hasn't been something that's happened, and its generally understood that headers should not have newlines in them..

The new record_http semantics make it easier to create WARC records from network traffic automatically..

Did you have something in mind specifically? My only thought is checking warc headers for trailing newlines perhaps.

wumpus commented 6 years ago

warc headers is one, http_headers is another to check. Since these are multi line things, it's easy to imagine a programmer screwing up and putting \r\n on the end. I think both of those would cause problems. I suppose I could write a test that demonstrates that...

dlazesz commented 5 years ago

I was reccently bitten by this problem in multiple ways:

Headers could end with space
Content can use \r\n line endings and end with an empty line

Both are stripped at some point when jugling with records thus changing the digests that were computed before the stripping and make the verification fail afterwards.

wumpus commented 5 years ago

I think those are separate bugs from what we were just discussing but bugs none the less -- can you provide a warc with records having these 'features' and a test that round trips it and complains that it's changed?