I've run into two related bugs with csrf_protection_p in ad_form
(packages/acs-tcl/tcl/form-processing-procs.tcl)
Bug 1 — Minor: csrf_protection_p set on an -extend call does not inject the CSRF token
If you set the flag on any call other than the first ad_form, the __csrf_token
hidden element is never added to the form. The element creation happens inside
the template::form create block, which only runs on the initial call:
ad_form -name my_form -form {
{item_id:key}
{name:text ...}
}
ad_form -extend -name my_form -csrf_protection_p 1 \
-edit_request {
} -new_data {
} -edit_data {
}
The -extend call with csrf_protection_p 1 is processed but the token element is never created. After the fix for Bug 2 (below), the workaround is to set csrf_protection_p in the first call. But is this expected? Should setting the flag on an extend retroactively inject the element?
Bug 2 — More severe: csrf_protection_p in the initial definition with a :key field breaks edit
When csrf_protection_p is specified in the first ad_form call (alongside a :key field), all subsequent -extend calls fail. The page errors with:
Key ... has the value ... but no select_query, select_query_name, or edit_request clause exists. (This can be caused by having ad_form request blocks in the wrong order.)
Example:
ad_form -name my_form \
-has_submit 1 \
-csrf_protection_p 1 \
-form {
{item_id:key}
{name:text {label "Name:"} {html {class "form-control"}}}
}
ad_form -extend -name my_form -form {
{description:text(textarea),optional {label "Description:"} ...}
}
ad_form -extend -name my_form \
-new_request {
set name ""
} -edit_request {
set name "Alice"
set description "Bob"
} -new_data {
} -edit_data {
} -after_submit { ad_returnredirect ...; ad_script_abort }
Root cause: In the foreach valid_arg loop in
packages/acs-tcl/tcl/form-processing-procs.tcl, any parameter not in the
"safe-to-extend" list immediately sets af_parts(${form_name}__extend). On the
very next -extend call, line 565 finds that flag set and aborts.
csrf_protection_p is not in that exclusion list:
if {$valid_arg ni {
name form method action html validate export mode cancel_url
has_edit has_submit actions edit_buttons display_buttons
fieldset on_validation_error
}} {
set af_parts(${form_name}__extend) ""
}
Proposed fix: Add csrf_protection_p (and show_required_p, which has the same
problem) to the list:
if {$valid_arg ni {
name form method action html validate export mode cancel_url
has_edit has_submit actions edit_buttons display_buttons
fieldset on_validation_error show_required_p csrf_protection_p
}} {
set af_parts(${form_name}__extend) ""
}
This fix resolves Bug 2. After applying it, Bug 1 is survivable by placing csrf_protection_p on the initial call — but it raises the question of whether setting it later should also work.
