=== modified file 'src/AccessLogEntry.h' --- src/AccessLogEntry.h 2012-10-29 04:59:58 +0000 +++ src/AccessLogEntry.h 2012-11-24 14:55:43 +0000 @@ -231,7 +231,9 @@ HttpReply *reply; HttpRequest *request; //< virgin HTTP request HttpRequest *adapted_request; //< HTTP request after adaptation and redirection - /// key:value pairs set by note and adaptation_meta + + /// key:value pairs set by note and adaptation_meta directives + /// plus key=value pairs returned from URL rewrite/redirect helper NotePairs notes; #if ICAP_CLIENT === modified file 'src/ClientRequestContext.h' --- src/ClientRequestContext.h 2012-11-04 12:27:49 +0000 +++ src/ClientRequestContext.h 2012-11-24 14:55:43 +0000 @@ -55,6 +55,7 @@ ClientHttpRequest *http; ACLChecklist *acl_checklist; /* need ptr back so we can unreg if needed */ int redirect_state; + uint8_t redirect_fail_count; bool host_header_verify_done; bool http_access_done; === modified file 'src/HttpRequest.cc' --- src/HttpRequest.cc 2012-10-27 00:13:19 +0000 +++ src/HttpRequest.cc 2012-11-24 14:55:43 +0000 @@ -114,6 +114,7 @@ peer_domain = NULL; // not allocated/deallocated by this class vary_headers = NULL; myportname = null_string; + metaNotes.clean(); tag = null_string; #if USE_AUTH extacl_user = null_string; @@ -221,6 +222,7 @@ // XXX: what to do with copy->peer_domain? copy->myportname = myportname; + copy->metaNotes.notes = metaNotes.notes; copy->tag = tag; #if USE_AUTH copy->extacl_user = extacl_user; === modified file 'src/HttpRequest.h' --- src/HttpRequest.h 2012-10-26 11:36:45 +0000 +++ src/HttpRequest.h 2012-11-24 14:55:43 +0000 @@ -38,6 +38,8 @@ #include "HttpMsg.h" #include "HttpRequestMethod.h" #include "RequestFlags.h" +#include "Notes.h" + #if USE_AUTH #include "auth/UserRequest.h" @@ -199,6 +201,8 @@ String myportname; // Internal tag name= value from port this requests arrived in. + Notes metaNotes; // collection of meta notes associated with this request. + String tag; /* Internal tag for this request */ String extacl_user; /* User name returned by extacl lookup */ === modified file 'src/Notes.cc' --- src/Notes.cc 2012-11-06 22:32:56 +0000 +++ src/Notes.cc 2012-11-24 14:55:43 +0000 @@ -102,6 +102,28 @@ return note; } +void +Notes::add(const Notes &src) +{ + typedef Notes::NotesList::const_iterator AMLI; + typedef Note::Values::iterator VLI; + + for (AMLI i = src.notes.begin(); i != src.notes.end(); ++i) { + + Note::Pointer ourKey = findByName((*i)->key); + // unknown key names are easy, add the whole Node to our list. + if (ourKey == NULL) { + notes.push_back(*i); + continue; + } + // known key names, merge the values lists... + for (VLI v = (*i)->values.begin(); v != (*i)->values.end(); ++v ) { + // TODO: prune duplicates ? + ourKey->values.push_back(*v); + } + } +} + Note::Pointer Notes::parse(ConfigParser &parser) { === modified file 'src/Notes.h' --- src/Notes.h 2012-11-06 22:32:56 +0000 +++ src/Notes.h 2012-11-24 14:55:43 +0000 @@ -103,6 +103,13 @@ void add(const String ¬eKey, const String ¬eValue); /** + * Adds a set of notes from another notes list to this set. + * Create any new keys needed. + * If the key name already exists in list, add the given value to its set of values. + */ + void add(const Notes &src); + + /** * Looks up a note by key name and returns a Note::Pointer to it */ Note::Pointer findByName(const String ¬eKey) const; @@ -112,6 +119,18 @@ { public: NotePairs() : HttpHeader(hoNote) {} + + /// convert a NotesList into a NotesPairs + /// appending to any existing entries already present + void append(const Notes::NotesList &src) { + for (Notes::NotesList::const_iterator m = src.begin(); m != src.end(); ++m) + for (Note::Values::iterator v =(*m)->values.begin(); v != (*m)->values.end(); ++v) + putExt((*m)->key.termedBuf(), (*v)->value.termedBuf()); + } + + void append(const NotePairs *src) { + HttpHeader::append(dynamic_cast(src)); + } }; #endif === modified file 'src/client_side.cc' --- src/client_side.cc 2012-11-08 10:49:58 +0000 +++ src/client_side.cc 2012-11-24 14:55:43 +0000 @@ -615,6 +615,7 @@ aLogEntry->http.method = request->method; aLogEntry->http.version = request->http_ver; aLogEntry->hier = request->hier; + aLogEntry->notes.append(request->metaNotes.notes); if (request->content_length > 0) // negative when no body or unknown length aLogEntry->cache.requestSize += request->content_length; aLogEntry->cache.extuser = request->extacl_user.termedBuf(); === modified file 'src/client_side_request.cc' --- src/client_side_request.cc 2012-11-08 08:49:33 +0000 +++ src/client_side_request.cc 2012-11-24 14:56:40 +0000 @@ -156,6 +156,7 @@ { http_access_done = false; redirect_done = false; + redirect_fail_count = 0; no_cache_done = false; interpreted_req_hdrs = false; #if USE_SSL @@ -1203,57 +1204,102 @@ assert(redirect_state == REDIRECT_PENDING); redirect_state = REDIRECT_DONE; - if (reply.other().hasContent()) { - /* 2012-06-28: This cast is due to urlParse() truncating too-long URLs itself. - * At this point altering the helper buffer in that way is not harmful, but annoying. - * When Bug 1961 is resolved and urlParse has a const API, this needs to die. - */ - char * result = const_cast(reply.other().content()); - http_status status = (http_status) atoi(result); - - if (status == HTTP_MOVED_PERMANENTLY - || status == HTTP_MOVED_TEMPORARILY - || status == HTTP_SEE_OTHER - || status == HTTP_PERMANENT_REDIRECT - || status == HTTP_TEMPORARY_REDIRECT) { - char *t = NULL; - - if ((t = strchr(result, ':')) != NULL) { + // copy the URL rewriter response Notes to the HTTP request for logging + // do it early to ensure that no matter what the outcome the notes are present. + // TODO put them straight into the transaction state record (ALE?) eventually + old_request->metaNotes.add(reply.responseKeys); + +// XXX update to handle the new format responses... +// #1: redirect with a specific status code OK status=NNN url="..." +// #2: redirect with a default status code OK url="..." +// #3: re-write the URL OK rewrite-url="..." + + switch(reply.result) { + case HelperReply::Unknown: + case HelperReply::TT: + debugs(85, DBG_IMPORTANT, "ERROR: URL rewrite helper returned invalid result code. Wrong helper? " << reply); + break; + + case HelperReply::BrokenHelper: + debugs(85, DBG_IMPORTANT, "ERROR: URL rewrite helper: " << reply); + if (redirect_fail_count < 2) { // XXX: make this configurable ? + ++redirect_fail_count; + // reset state flag to try redirector again from scratch. + redirect_done = false; + } + break; + + case HelperReply::Error: + // no change to be done. + break; + + case HelperReply::Okay: { + + Note::Pointer statusNote = reply.responseKeys.findByName("status"); + Note::Pointer urlNote = reply.responseKeys.findByName("url"); + + if (urlNote != NULL) { + // HTTP protocol redirect to be done. + + // TODO: change default redirect status for appropriate requests + // Squid defaults to 302 status for now for better compatibility with old clients. + // HTTP/1.0 client should get 302 (HTTP_MOVED_TEMPORARILY) + // HTTP/1.1 client contacting reverse-proxy should get 307 (HTTP_TEMPORARY_REDIRECT) + // HTTP/1.1 client being diverted by forward-proxy should get 303 (HTTP_SEE_OTHER) + http_status status = HTTP_MOVED_TEMPORARILY; + if (statusNote != NULL) { + char * result = const_cast(statusNote->values[0]->value.termedBuf()); + status = (http_status) atoi(result); + } + + if (status == HTTP_MOVED_PERMANENTLY + || status == HTTP_MOVED_TEMPORARILY + || status == HTTP_SEE_OTHER + || status == HTTP_PERMANENT_REDIRECT + || status == HTTP_TEMPORARY_REDIRECT) { http->redirect.status = status; - http->redirect.location = xstrdup(t + 1); + http->redirect.location = xstrdup(urlNote->values[0]->value.termedBuf()); // TODO: validate the URL produced here is RFC 2616 compliant absolute URI } else { - debugs(85, DBG_CRITICAL, "ERROR: URL-rewrite produces invalid " << status << " redirect Location: " << result); + debugs(85, DBG_CRITICAL, "ERROR: URL-rewrite produces invalid " << status << " redirect Location: " << urlNote->values[0]->value); } - } else if (strcmp(result, http->uri)) { - // XXX: validate the URL properly *without* generating a whole new request object right here. - // XXX: the clone() should be done only AFTER we know the new URL is valid. - HttpRequest *new_request = old_request->clone(); - if (urlParse(old_request->method, result, new_request)) { - debugs(61,2, HERE << "URL-rewriter diverts URL from " << urlCanonical(old_request) << " to " << urlCanonical(new_request)); - - // update the new request to flag the re-writing was done on it - new_request->flags.redirected = 1; - - // unlink bodypipe from the old request. Not needed there any longer. - if (old_request->body_pipe != NULL) { - old_request->body_pipe = NULL; - debugs(61,2, HERE << "URL-rewriter diverts body_pipe " << new_request->body_pipe << - " from request " << old_request << " to " << new_request); + } else { + // URL-rewrite wanted. Ew. + urlNote = reply.responseKeys.findByName("rewrite-url"); + + // prevent broken helpers causing too much damage. If old URL == new URL skip the re-write. + if (urlNote != NULL && strcmp(urlNote->values[0]->value.termedBuf(), http->uri)) { + // XXX: validate the URL properly *without* generating a whole new request object right here. + // XXX: the clone() should be done only AFTER we know the new URL is valid. + HttpRequest *new_request = old_request->clone(); + if (urlParse(old_request->method, const_cast(urlNote->values[0]->value.termedBuf()), new_request)) { + debugs(61,2, HERE << "URL-rewriter diverts URL from " << urlCanonical(old_request) << " to " << urlCanonical(new_request)); + + // update the new request to flag the re-writing was done on it + new_request->flags.redirected = 1; + + // unlink bodypipe from the old request. Not needed there any longer. + if (old_request->body_pipe != NULL) { + old_request->body_pipe = NULL; + debugs(61,2, HERE << "URL-rewriter diverts body_pipe " << new_request->body_pipe << + " from request " << old_request << " to " << new_request); + } + + // update the current working ClientHttpRequest fields + safe_free(http->uri); + http->uri = xstrdup(urlCanonical(new_request)); + HTTPMSGUNLOCK(old_request); + http->request = HTTPMSGLOCK(new_request); + } else { + debugs(85, DBG_CRITICAL, "ERROR: URL-rewrite produces invalid request: " << + old_request->method << " " << urlNote->values[0]->value << " " << old_request->http_ver); + delete new_request; } - - // update the current working ClientHttpRequest fields - safe_free(http->uri); - http->uri = xstrdup(urlCanonical(new_request)); - HTTPMSGUNLOCK(old_request); - http->request = HTTPMSGLOCK(new_request); - } else { - debugs(85, DBG_CRITICAL, "ERROR: URL-rewrite produces invalid request: " << - old_request->method << " " << result << " " << old_request->http_ver); - delete new_request; } } } + break; + } /* FIXME PIPELINE: This is innacurate during pipelining */ === modified file 'src/redirect.cc' --- src/redirect.cc 2012-11-08 08:49:33 +0000 +++ src/redirect.cc 2012-11-24 14:55:43 +0000 @@ -78,8 +78,9 @@ redirectStateData *r = static_cast(data); debugs(61, 5, HERE << "reply=" << reply); - // XXX: This funtion is now kept only to check for and display this garbage use-case - // it can be removed when the helpers are all updated to the normalized "OK/ERR key-pairs" format + // XXX: This function is now kept only to check for and display the garbage use-case + // and to map the old helper response format(s) into new format result code and key=value pairs + // it can be removed when the helpers are all updated to the normalized "OK/ERR kv-pairs" format if (reply.result == HelperReply::Unknown) { // BACKWARD COMPATIBILITY 2012-06-15: @@ -99,6 +100,52 @@ } if (reply.other().hasContent() && *res == '\0') reply.modifiableOther().clean(); // drop the whole buffer of garbage. + + // if we still have anything in other() after all that + // parse it into status=, url= and rewrite-url= keys + if (reply.other().hasContent()) { + /* 2012-06-28: This cast is due to urlParse() truncating too-long URLs itself. + * At this point altering the helper buffer in that way is not harmful, but annoying. + * When Bug 1961 is resolved and urlParse has a const API, this needs to die. + */ + char * result = const_cast(reply.other().content()); + http_status status = (http_status) atoi(result); + + HelperReply newReply; + newReply.result = reply.result; + newReply.responseKeys = reply.responseKeys; + + if (status == HTTP_MOVED_PERMANENTLY + || status == HTTP_MOVED_TEMPORARILY + || status == HTTP_SEE_OTHER + || status == HTTP_PERMANENT_REDIRECT + || status == HTTP_TEMPORARY_REDIRECT) { + char *t = NULL; + + if ((t = strchr(result, ':')) != NULL) { + char statusBuf[4]; + snprintf(statusBuf, sizeof(statusBuf),"%3u",status); + newReply.responseKeys.add("status", statusBuf); + ++t; + // TODO: validate the URL produced here is RFC 2616 compliant URI + newReply.responseKeys.add("url", t); + } else { + debugs(85, DBG_CRITICAL, "ERROR: URL-rewrite produces invalid " << status << " redirect Location: " << result); + } + } else { + // status code is not a redirect code (or does not exist) + // treat as a re-write URL request + // TODO: validate the URL produced here is RFC 2616 compliant URI + newReply.responseKeys.add("rewrite-url", reply.other().content()); + } + + void *cbdata; + if (cbdataReferenceValidDone(r->data, &cbdata)) + r->handler(cbdata, newReply); + + redirectStateFree(r); + return; + } } }