From mboxrd@z Thu Jan 1 00:00:00 1970 From: john at keeping.me.uk (John Keeping) Date: Fri, 15 Nov 2013 14:09:58 +0000 Subject: [PATCH] Return HTTP status codes for error conditions for various commands In-Reply-To: <1384524085-15759-1-git-send-email-mail@dirk-best.de> References: <1384524085-15759-1-git-send-email-mail@dirk-best.de> Message-ID: <20131115140958.GU24023@serenity.lan> On Fri, Nov 15, 2013 at 03:01:25PM +0100, Dirk Best wrote: > This returns 404 Not found for most errors, the only exception is for the stats, where it will return 400 > Bad request for unknown statistics types. Note that this patch also fixes various incomplete header errors, > which manifest themselves like this in the web server error log: "Premature end of script headers: > cgit.cgi". > > Signed-off-by: Dirk Best > --- This change looks fairly sensible from a quick scan through, but there are several different changes rolled into a single patch here. At the very least I think there are three commits in here: 1/3 Add cgit_print_pagestart() helper function Refactor existing code to reduce duplication. 2/3 Remove want_layout from command structure Move print_pagestart and print_docend into individual commands, no change in functionality. 3/3 Add error messages Change in functionality. > cgit.c | 33 +++++---------------------------- > cmd.c | 58 ++++++++++++++++++++++++++++++++++------------------------ > cmd.h | 1 - > ui-commit.c | 11 ++++++++++- > ui-diff.c | 20 +++++++++++++++++--- > ui-diff.h | 2 +- > ui-patch.c | 8 ++++++++ > ui-shared.c | 16 ++++++++++++++++ > ui-shared.h | 3 +++ > ui-stats.c | 7 +++++++ > ui-summary.c | 5 +++++ > ui-tag.c | 16 +++++++++++++++- > ui-tree.c | 8 ++++++++ > 13 files changed, 129 insertions(+), 59 deletions(-) > > diff --git a/cgit.c b/cgit.c > index 861352a..f1ba6dd 100644 > --- a/cgit.c > +++ b/cgit.c > @@ -575,9 +575,7 @@ static int prepare_repo_cmd(struct cgit_context *ctx) > ctx->page.title = fmtalloc("%s - %s", ctx->cfg.root_title, > "config error"); > ctx->repo = NULL; > - cgit_print_http_headers(ctx); > - cgit_print_docstart(ctx); > - cgit_print_pageheader(ctx); > + cgit_print_pagestart(ctx); > cgit_print_error("Failed to open %s: %s", name, > rc ? strerror(rc) : "Not a valid git repository"); > cgit_print_docend(); > @@ -594,9 +592,7 @@ static int prepare_repo_cmd(struct cgit_context *ctx) > } > > if (!ctx->qry.head) { > - cgit_print_http_headers(ctx); > - cgit_print_docstart(ctx); > - cgit_print_pageheader(ctx); > + cgit_print_pagestart(ctx); > cgit_print_error("Repository seems to be empty"); > cgit_print_docend(); > return 1; > @@ -605,11 +601,7 @@ static int prepare_repo_cmd(struct cgit_context *ctx) > if (get_sha1(ctx->qry.head, sha1)) { > char *tmp = xstrdup(ctx->qry.head); > ctx->qry.head = ctx->repo->defbranch; > - ctx->page.status = 404; > - ctx->page.statusmsg = "Not found"; > - cgit_print_http_headers(ctx); > - cgit_print_docstart(ctx); > - cgit_print_pageheader(ctx); > + cgit_print_pagestart_error(ctx, 404, "Not found"); > cgit_print_error("Invalid branch: %s", tmp); > cgit_print_docend(); > return 1; > @@ -628,11 +620,7 @@ static void process_request(void *cbdata) > cmd = cgit_get_cmd(ctx); > if (!cmd) { > ctx->page.title = "cgit error"; > - ctx->page.status = 404; > - ctx->page.statusmsg = "Not found"; > - cgit_print_http_headers(ctx); > - cgit_print_docstart(ctx); > - cgit_print_pageheader(ctx); > + cgit_print_pagestart_error(ctx, 404, "Not found"); > cgit_print_error("Invalid request"); > cgit_print_docend(); > return; > @@ -650,9 +638,7 @@ static void process_request(void *cbdata) > ctx->qry.vpath = cmd->want_vpath ? ctx->qry.path : NULL; > > if (cmd->want_repo && !ctx->repo) { > - cgit_print_http_headers(ctx); > - cgit_print_docstart(ctx); > - cgit_print_pageheader(ctx); > + cgit_print_pagestart(ctx); > cgit_print_error("No repository selected"); > cgit_print_docend(); > return; > @@ -661,16 +647,7 @@ static void process_request(void *cbdata) > if (ctx->repo && prepare_repo_cmd(ctx)) > return; > > - if (cmd->want_layout) { > - cgit_print_http_headers(ctx); > - cgit_print_docstart(ctx); > - cgit_print_pageheader(ctx); > - } > - > cmd->fn(ctx); > - > - if (cmd->want_layout) > - cgit_print_docend(); > } > > static int cmp_repos(const void *a, const void *b) > diff --git a/cmd.c b/cmd.c > index 0202917..d414450 100644 > --- a/cmd.c > +++ b/cmd.c > @@ -39,10 +39,14 @@ static void atom_fn(struct cgit_context *ctx) > > static void about_fn(struct cgit_context *ctx) > { > + cgit_print_pagestart(ctx); > + > if (ctx->repo) > cgit_print_repo_readme(ctx->qry.path); > else > cgit_print_site_readme(); > + > + cgit_print_docend(); > } > > static void blob_fn(struct cgit_context *ctx) > @@ -57,12 +61,12 @@ static void commit_fn(struct cgit_context *ctx) > > static void diff_fn(struct cgit_context *ctx) > { > - cgit_print_diff(ctx->qry.sha1, ctx->qry.sha2, ctx->qry.path, 1, 0); > + cgit_print_diff(ctx->qry.sha1, ctx->qry.sha2, ctx->qry.path, 1, 0, 1); > } > > static void rawdiff_fn(struct cgit_context *ctx) > { > - cgit_print_diff(ctx->qry.sha1, ctx->qry.sha2, ctx->qry.path, 1, 1); > + cgit_print_diff(ctx->qry.sha1, ctx->qry.sha2, ctx->qry.path, 1, 1, 1); > } > > static void info_fn(struct cgit_context *ctx) > @@ -72,10 +76,14 @@ static void info_fn(struct cgit_context *ctx) > > static void log_fn(struct cgit_context *ctx) > { > + cgit_print_pagestart(ctx); > + > cgit_print_log(ctx->qry.sha1, ctx->qry.ofs, ctx->cfg.max_commit_count, > ctx->qry.grep, ctx->qry.search, ctx->qry.path, 1, > ctx->repo->enable_commit_graph, > ctx->repo->commit_sort); > + > + cgit_print_docend(); > } > > static void ls_cache_fn(struct cgit_context *ctx) > @@ -108,7 +116,9 @@ static void plain_fn(struct cgit_context *ctx) > > static void refs_fn(struct cgit_context *ctx) > { > + cgit_print_pagestart(ctx); > cgit_print_refs(); > + cgit_print_docend(); > } > > static void snapshot_fn(struct cgit_context *ctx) > @@ -137,32 +147,32 @@ static void tree_fn(struct cgit_context *ctx) > cgit_print_tree(ctx->qry.sha1, ctx->qry.path); > } > > -#define def_cmd(name, want_repo, want_layout, want_vpath, is_clone) \ > - {#name, name##_fn, want_repo, want_layout, want_vpath, is_clone} > +#define def_cmd(name, want_repo, want_vpath, is_clone) \ > + {#name, name##_fn, want_repo, want_vpath, is_clone} > > struct cgit_cmd *cgit_get_cmd(struct cgit_context *ctx) > { > static struct cgit_cmd cmds[] = { > - def_cmd(HEAD, 1, 0, 0, 1), > - def_cmd(atom, 1, 0, 0, 0), > - def_cmd(about, 0, 1, 0, 0), > - def_cmd(blob, 1, 0, 0, 0), > - def_cmd(commit, 1, 1, 1, 0), > - def_cmd(diff, 1, 1, 1, 0), > - def_cmd(info, 1, 0, 0, 1), > - def_cmd(log, 1, 1, 1, 0), > - def_cmd(ls_cache, 0, 0, 0, 0), > - def_cmd(objects, 1, 0, 0, 1), > - def_cmd(patch, 1, 0, 1, 0), > - def_cmd(plain, 1, 0, 0, 0), > - def_cmd(rawdiff, 1, 0, 1, 0), > - def_cmd(refs, 1, 1, 0, 0), > - def_cmd(repolist, 0, 0, 0, 0), > - def_cmd(snapshot, 1, 0, 0, 0), > - def_cmd(stats, 1, 1, 1, 0), > - def_cmd(summary, 1, 1, 0, 0), > - def_cmd(tag, 1, 1, 0, 0), > - def_cmd(tree, 1, 1, 1, 0), > + def_cmd(HEAD, 1, 0, 1), > + def_cmd(atom, 1, 0, 0), > + def_cmd(about, 0, 0, 0), > + def_cmd(blob, 1, 0, 0), > + def_cmd(commit, 1, 1, 0), > + def_cmd(diff, 1, 1, 0), > + def_cmd(info, 1, 0, 1), > + def_cmd(log, 1, 1, 0), > + def_cmd(ls_cache, 0, 0, 0), > + def_cmd(objects, 1, 0, 1), > + def_cmd(patch, 1, 1, 0), > + def_cmd(plain, 1, 0, 0), > + def_cmd(rawdiff, 1, 1, 0), > + def_cmd(refs, 1, 0, 0), > + def_cmd(repolist, 0, 0, 0), > + def_cmd(snapshot, 1, 0, 0), > + def_cmd(stats, 1, 1, 0), > + def_cmd(summary, 1, 0, 0), > + def_cmd(tag, 1, 0, 0), > + def_cmd(tree, 1, 1, 0), > }; > int i; > > diff --git a/cmd.h b/cmd.h > index eb5bc87..8c88b32 100644 > --- a/cmd.h > +++ b/cmd.h > @@ -7,7 +7,6 @@ struct cgit_cmd { > const char *name; > cgit_cmd_fn fn; > unsigned int want_repo:1, > - want_layout:1, > want_vpath:1, > is_clone:1; > }; > diff --git a/ui-commit.c b/ui-commit.c > index ef85a49..d29039d 100644 > --- a/ui-commit.c > +++ b/ui-commit.c > @@ -27,14 +27,21 @@ void cgit_print_commit(char *hex, const char *prefix) > hex = ctx.qry.head; > > if (get_sha1(hex, sha1)) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Bad object id: %s", hex); > + cgit_print_docend(); > return; > } > commit = lookup_commit_reference(sha1); > if (!commit) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Bad commit reference: %s", hex); > + cgit_print_docend(); > return; > } > + > + cgit_print_pagestart(&ctx); > + > info = cgit_parse_commit(commit); > > format_display_notes(sha1, ¬es, PAGE_ENCODING, 0); > @@ -137,8 +144,10 @@ void cgit_print_commit(char *hex, const char *prefix) > tmp = sha1_to_hex(commit->parents->item->object.sha1); > else > tmp = NULL; > - cgit_print_diff(ctx.qry.sha1, tmp, prefix, 0, 0); > + cgit_print_diff(ctx.qry.sha1, tmp, prefix, 0, 0, 0); > } > strbuf_release(¬es); > cgit_free_commitinfo(info); > + > + cgit_print_docend(); > } > diff --git a/ui-diff.c b/ui-diff.c > index 1209c47..000c794 100644 > --- a/ui-diff.c > +++ b/ui-diff.c > @@ -358,25 +358,31 @@ void cgit_print_diff_ctrls() > } > > void cgit_print_diff(const char *new_rev, const char *old_rev, > - const char *prefix, int show_ctrls, int raw) > + const char *prefix, int show_ctrls, int raw, int full_page) > { > struct commit *commit, *commit2; > > if (!new_rev) > new_rev = ctx.qry.head; > if (get_sha1(new_rev, new_rev_sha1)) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Bad object name: %s", new_rev); > - return; > + cgit_print_docend(); > + return; > } > commit = lookup_commit_reference(new_rev_sha1); > if (!commit || parse_commit(commit)) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Bad commit: %s", sha1_to_hex(new_rev_sha1)); > - return; > + cgit_print_docend(); > + return; > } > > if (old_rev) { > if (get_sha1(old_rev, old_rev_sha1)) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Bad object name: %s", old_rev); > + cgit_print_docend(); > return; > } > } else if (commit->parents && commit->parents->item) { > @@ -388,7 +394,9 @@ void cgit_print_diff(const char *new_rev, const char *old_rev, > if (!is_null_sha1(old_rev_sha1)) { > commit2 = lookup_commit_reference(old_rev_sha1); > if (!commit2 || parse_commit(commit2)) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Bad commit: %s", sha1_to_hex(old_rev_sha1)); > + cgit_print_docend(); > return; > } > } > @@ -401,6 +409,9 @@ void cgit_print_diff(const char *new_rev, const char *old_rev, > return; > } > > + if (full_page) > + cgit_print_pagestart(&ctx); > + > use_ssdiff = ctx.qry.has_ssdiff ? ctx.qry.ssdiff : ctx.cfg.ssdiff; > > if (show_ctrls) > @@ -419,4 +430,7 @@ void cgit_print_diff(const char *new_rev, const char *old_rev, > if (!use_ssdiff) > html(""); > html(""); > + > + if (full_page) > + cgit_print_docend(); > } > diff --git a/ui-diff.h b/ui-diff.h > index 04f9029..dac7a59 100644 > --- a/ui-diff.h > +++ b/ui-diff.h > @@ -4,7 +4,7 @@ > extern void cgit_print_diff_ctrls(); > > extern void cgit_print_diff(const char *new_hex, const char *old_hex, > - const char *prefix, int show_ctrls, int raw); > + const char *prefix, int show_ctrls, int raw, int full_page); > > extern struct diff_filespec *cgit_get_current_old_file(void); > extern struct diff_filespec *cgit_get_current_new_file(void); > diff --git a/ui-patch.c b/ui-patch.c > index 4d35167..9aec0a3 100644 > --- a/ui-patch.c > +++ b/ui-patch.c > @@ -25,22 +25,30 @@ void cgit_print_patch(const char *new_rev, const char *old_rev, > new_rev = ctx.qry.head; > > if (get_sha1(new_rev, new_rev_sha1)) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Bad object id: %s", new_rev); > + cgit_print_docend(); > return; > } > commit = lookup_commit_reference(new_rev_sha1); > if (!commit) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Bad commit reference: %s", new_rev); > + cgit_print_docend(); > return; > } > > if (old_rev) { > if (get_sha1(old_rev, old_rev_sha1)) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Bad object id: %s", old_rev); > + cgit_print_docend(); > return; > } > if (!lookup_commit_reference(old_rev_sha1)) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Bad commit reference: %s", old_rev); > + cgit_print_docend(); > return; > } > } else if (commit->parents && commit->parents->item) { > diff --git a/ui-shared.c b/ui-shared.c > index 1e19421..2c0cac5 100644 > --- a/ui-shared.c > +++ b/ui-shared.c > @@ -696,6 +696,22 @@ void cgit_print_docstart(struct cgit_context *ctx) > html_include(ctx->cfg.header); > } > > +void cgit_print_pagestart(struct cgit_context *ctx) > +{ > + cgit_print_http_headers(ctx); > + cgit_print_docstart(ctx); > + cgit_print_pageheader(ctx); > +} > + > +void cgit_print_pagestart_error(struct cgit_context *ctx, int status, const char *reason) > +{ > + ctx->page.status = status; > + ctx->page.statusmsg = reason; > + cgit_print_http_headers(ctx); > + cgit_print_docstart(ctx); > + cgit_print_pageheader(ctx); > +} > + > void cgit_print_docend() > { > html(" \n"); > diff --git a/ui-shared.h b/ui-shared.h > index a337dce..4110f83 100644 > --- a/ui-shared.h > +++ b/ui-shared.h > @@ -60,6 +60,9 @@ extern void cgit_print_date(time_t secs, const char *format, int local_time); > extern void cgit_print_age(time_t t, time_t max_relative, const char *format); > extern void cgit_print_http_headers(struct cgit_context *ctx); > extern void cgit_print_docstart(struct cgit_context *ctx); > +extern void cgit_print_pagestart(struct cgit_context *ctx); > +extern void cgit_print_pagestart_error(struct cgit_context *ctx, int status, > + const char *reason); > extern void cgit_print_docend(); > extern void cgit_print_pageheader(struct cgit_context *ctx); > extern void cgit_print_filemode(unsigned short mode); > diff --git a/ui-stats.c b/ui-stats.c > index 28b794f..cefea9f 100644 > --- a/ui-stats.c > +++ b/ui-stats.c > @@ -374,9 +374,14 @@ void cgit_show_stats(struct cgit_context *ctx) > > i = cgit_find_stats_period(code, &period); > if (!i) { > + cgit_print_pagestart_error(ctx, 400, "Bad request"); > cgit_print_error("Unknown statistics type: %c", code[0]); > + cgit_print_docend(); > return; > } > + > + cgit_print_pagestart(ctx); > + > if (i > ctx->repo->max_stats) { > cgit_print_error("Statistics type disabled: %s", period->name); > return; > @@ -423,5 +428,7 @@ void cgit_show_stats(struct cgit_context *ctx) > } > html(""); > print_authors(&authors, top, period); > + > + cgit_print_docend(); > } > > diff --git a/ui-summary.c b/ui-summary.c > index 5598d08..8c15980 100644 > --- a/ui-summary.c > +++ b/ui-summary.c > @@ -13,6 +13,7 @@ > #include "ui-log.h" > #include "ui-refs.h" > #include "ui-blob.h" > +#include "ui-shared.h" > #include > > static void print_url(char *base, char *suffix) > @@ -80,6 +81,8 @@ void cgit_print_summary() > if (ctx.repo->enable_log_linecount) > columns++; > > + cgit_print_pagestart(&ctx); > + > html(""); > cgit_print_branches(ctx.cfg.summary_branches); > htmlf("", columns); > @@ -94,6 +97,8 @@ void cgit_print_summary() > else if (ctx.cfg.clone_prefix) > print_urls(ctx.cfg.clone_prefix, ctx.repo->url); > html("
 
"); > + > + cgit_print_docend(); > } > > /* The caller must free the return value. */ > diff --git a/ui-tag.c b/ui-tag.c > index aea7958..103f9be 100644 > --- a/ui-tag.c > +++ b/ui-tag.c > @@ -44,7 +44,7 @@ void cgit_print_tag(char *revname) > struct strbuf fullref = STRBUF_INIT; > unsigned char sha1[20]; > struct object *obj; > - struct tag *tag; > + struct tag *tag = NULL; > struct taginfo *info; > > if (!revname) > @@ -52,20 +52,32 @@ void cgit_print_tag(char *revname) > > strbuf_addf(&fullref, "refs/tags/%s", revname); > if (get_sha1(fullref.buf, sha1)) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Bad tag reference: %s", revname); > + cgit_print_docend(); > goto cleanup; > } > obj = parse_object(sha1); > if (!obj) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Bad object id: %s", sha1_to_hex(sha1)); > + cgit_print_docend(); > goto cleanup; > } > + > if (obj->type == OBJ_TAG) { > tag = lookup_tag(sha1); > if (!tag || parse_tag(tag) || !(info = cgit_parse_tag(tag))) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Bad tag object: %s", revname); > + cgit_print_docend(); > goto cleanup; > } > + } > + > + cgit_print_pagestart(&ctx); > + > + if (tag) { > html("\n"); > htmlf("
tag name"); > html_txt(revname); > @@ -104,6 +116,8 @@ void cgit_print_tag(char *revname) > html("
\n"); > } > > + cgit_print_docend(); > + > cleanup: > strbuf_release(&fullref); > } > diff --git a/ui-tree.c b/ui-tree.c > index aa5dee9..0074a69 100644 > --- a/ui-tree.c > +++ b/ui-tree.c > @@ -271,15 +271,21 @@ void cgit_print_tree(const char *rev, char *path) > rev = ctx.qry.head; > > if (get_sha1(rev, sha1)) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Invalid revision name: %s", rev); > + cgit_print_docend(); > return; > } > commit = lookup_commit_reference(sha1); > if (!commit || parse_commit(commit)) { > + cgit_print_pagestart_error(&ctx, 404, "Not found"); > cgit_print_error("Invalid commit reference: %s", rev); > + cgit_print_docend(); > return; > } > > + cgit_print_pagestart(&ctx); > + > walk_tree_ctx.curr_rev = xstrdup(rev); > > if (path == NULL) { > @@ -293,4 +299,6 @@ void cgit_print_tree(const char *rev, char *path) > > cleanup: > free(walk_tree_ctx.curr_rev); > + > + cgit_print_docend(); > } > -- > 1.8.3.2 > > _______________________________________________ > CGit mailing list > CGit at lists.zx2c4.com > http://lists.zx2c4.com/mailman/listinfo/cgit